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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <alpine.LFD.2.20.1707151520150.24574@casper.infradead.org>
Date:   Sat, 15 Jul 2017 15:40:29 +0100 (BST)
From:   James Simmons <jsimmons@...radead.org>
To:     Al Viro <viro@...IV.linux.org.uk>
cc:     Arnd Bergmann <arnd@...db.de>, 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>,
        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 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.