linux-kernel - Re: [PATCH] lustre: check copy_from_iter/copy_to

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 13 Jul 2017 22:57:59 +0200
From:   Arnd Bergmann <arnd@...db.de>
To:     James Simmons <jsimmons@...radead.org>
Cc:     Oleg Drokin <oleg.drokin@...el.com>,
        Andreas Dilger <andreas.dilger@...el.com>,
        "# 3.4.x" <stable@...r.kernel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Doug Oucharek <doug.s.oucharek@...el.com>,
        Dmitry Eremin <dmitry.eremin@...el.com>,
        Al Viro <viro@...iv.linux.org.uk>,
        Liang Zhen <liang.zhen@...el.com>,
        Nicholas Hanley <nicholasjhanley@...il.com>,
        Lustre Development List <lustre-devel@...ts.lustre.org>,
        devel@...verdev.osuosl.org,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] lustre: check copy_from_iter/copy_to_iter return code

On Thu, Jul 13, 2017 at 7:07 PM, James Simmons <jsimmons@...radead.org> wrote:
>
>> 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@...r.kernel.org
>> Signed-off-by: Arnd Bergmann <arnd@...db.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(-)
>>
>> 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);
>
> I have to Nak this to prevent it from landing but for some reason in my
> testing its returning zero from copy_from_iter.

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?

        Arnd