lustre: check copy_from_iter/copy_to_iter return code

Message ID 20170710130833.1834210-1-arnd@arndb.de
State New
Headers show

Commit Message

Arnd Bergmann July 10, 2017, 1:08 p.m.
We now get a helpful warning for code that calls copy_{from,to}_iter
without checking the return value, introduced by commit aa28de275a24
("iov_iter/hardening: move object size checks to inlined part").

drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd_cb.c: In function 'kiblnd_send':
drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd_cb.c:1643:2: error: ignoring return value of 'copy_from_iter', declared with attribute warn_unused_result [-Werror=unused-result]
drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd_cb.c: In function 'kiblnd_recv':
drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd_cb.c:1744:3: error: ignoring return value of 'copy_to_iter', declared with attribute warn_unused_result [-Werror=unused-result]

In case we get short copies here, we may get incorrect behavior.
I've added failure handling for both rx and tx now, returning
-EFAULT as expected.

Cc: stable@vger.kernel.org
Signed-off-by: Arnd Bergmann <arnd@arndb.de>

---
This warning now shows up in 'allmodconfig' builds, so it would be
good to get it fixed quickly for 4.13, but my patch should not go
in without careful review since I'm not familiar with with code
and the error handling is a bit tricky here.

I added 'Cc: stable' since this is a preexisting condition that we
only started warning about now.
---
 drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd_cb.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

-- 
2.9.0

Comments

Al Viro July 14, 2017, 1:50 a.m. | #1
On Thu, Jul 13, 2017 at 10:57:59PM +0200, Arnd Bergmann wrote:

> Thanks for testing it!

> 

> That means we did not copy any data and the kernel continues with

> an uninitialized buffer, right? The problem may be the definition of

> 

> struct kib_immediate_msg {

>         struct lnet_hdr ibim_hdr;        /* portals header */

>         char         ibim_payload[0]; /* piggy-backed payload */

> } WIRE_ATTR;

> 

> The check that Al added will try to ensure that we don't write

> beyond the size of the ibim_payload[] array, which unfortunately

> is defined as a zero-byte array, so I can see why it will now

> fail. However, it's already broken in mainline now, with or without

> my patch.

> 

> Are you able to come up with a fix that avoids the warning in

> 'allmodconfig' and makes the function do something reasonable

> again?


Might make sense to try and use valid C99 for "array of indefinite
size as the last member", i.e.
struct kib_immediate_msg {
         struct lnet_hdr ibim_hdr;        /* portals header */
         char         ibim_payload[]; /* piggy-backed payload */
} WIRE_ATTR;

	Zero-sized array as the last member is gcc hack predating that;
looks like gcc gets confused into deciding that it knows the distance
from the end of object...

	Said that, are we really guaranteed the IBLND_MSG_SIZE bytes
in there?
James Simmons July 15, 2017, 2:40 p.m. | #2
On Fri, 14 Jul 2017, Al Viro wrote:

> On Thu, Jul 13, 2017 at 10:57:59PM +0200, Arnd Bergmann wrote:

> 

> > Thanks for testing it!

> > 

> > That means we did not copy any data and the kernel continues with

> > an uninitialized buffer, right? The problem may be the definition of

> > 

> > struct kib_immediate_msg {

> >         struct lnet_hdr ibim_hdr;        /* portals header */

> >         char         ibim_payload[0]; /* piggy-backed payload */

> > } WIRE_ATTR;

> > 

> > The check that Al added will try to ensure that we don't write

> > beyond the size of the ibim_payload[] array, which unfortunately

> > is defined as a zero-byte array, so I can see why it will now

> > fail. However, it's already broken in mainline now, with or without

> > my patch.

> > 

> > Are you able to come up with a fix that avoids the warning in

> > 'allmodconfig' and makes the function do something reasonable

> > again?


Yes, I'm testing a fix right now which I will merge with the original
patch. Greg this patch will need to be sent to Linus as well so the
kernel release isn't broken for users.
 
> Might make sense to try and use valid C99 for "array of indefinite

> size as the last member", i.e.

> struct kib_immediate_msg {

>          struct lnet_hdr ibim_hdr;        /* portals header */

>          char         ibim_payload[]; /* piggy-backed payload */

> } WIRE_ATTR;

> 

> 	Zero-sized array as the last member is gcc hack predating that;

> looks like gcc gets confused into deciding that it knows the distance

> from the end of object...


I did some profiling and found gcc was doing the right thing. That
should be updated to a C99 flexable array in a latter patch. 

> 	Said that, are we really guaranteed the IBLND_MSG_SIZE bytes

> in there?


This is what the real bug was. In the current code we are telling
copy_from_iter and copy_to_iter that the number of bytes are always
IBLND_MSG_SIZE. Arnd thought this was always the size so in his
patch he was testing the returned result of copy_[from|to]_iter to 
IBLND_MSG_SIZE. This nearly always failed since variable sized messages 
are being created. The zero size I initially saw was from doing pings. 
When I later tested with pushing I/O packets of other sizes were
observed but none of them were IBLND_MSG_SIZE in size so they failed to 
transmit. As soon as I'm done testing I will send a patch.

Patch

diff --git a/drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd_cb.c b/drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd_cb.c
index 85b242ec5f9b..70256a0ffd2e 100644
--- a/drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd_cb.c
+++ b/drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd_cb.c
@@ -1640,8 +1640,13 @@  kiblnd_send(struct lnet_ni *ni, void *private, struct lnet_msg *lntmsg)
 	ibmsg = tx->tx_msg;
 	ibmsg->ibm_u.immediate.ibim_hdr = *hdr;
 
-	copy_from_iter(&ibmsg->ibm_u.immediate.ibim_payload, IBLND_MSG_SIZE,
+	rc = copy_from_iter(&ibmsg->ibm_u.immediate.ibim_payload, IBLND_MSG_SIZE,
 		       &from);
+	if (rc != IBLND_MSG_SIZE) {
+		kiblnd_pool_free_node(&tx->tx_pool->tpo_pool, &tx->tx_list);
+		return -EFAULT;
+	}
+
 	nob = offsetof(struct kib_immediate_msg, ibim_payload[payload_nob]);
 	kiblnd_init_tx_msg(ni, tx, IBLND_MSG_IMMEDIATE, nob);
 
@@ -1741,8 +1746,14 @@  kiblnd_recv(struct lnet_ni *ni, void *private, struct lnet_msg *lntmsg,
 			break;
 		}
 
-		copy_to_iter(&rxmsg->ibm_u.immediate.ibim_payload,
+		rc = copy_to_iter(&rxmsg->ibm_u.immediate.ibim_payload,
 			     IBLND_MSG_SIZE, to);
+		if (rc != IBLND_MSG_SIZE) {
+			rc = -EFAULT;
+			break;
+		}
+
+		rc = 0;
 		lnet_finalize(ni, lntmsg, 0);
 		break;