linux-kernel - Re: [PATCH] infiniband: avoid overflow warning

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <1501537079.1180.3.camel@gmail.com>
Date:   Mon, 31 Jul 2017 17:37:59 -0400
From:   Daniel Micay <danielmicay@...il.com>
To:     Kees Cook <keescook@...omium.org>, Arnd Bergmann <arnd@...db.de>
Cc:     Moni Shoua <monis@...lanox.com>,
        Doug Ledford <dledford@...hat.com>,
        Sean Hefty <sean.hefty@...el.com>,
        Hal Rosenstock <hal.rosenstock@...il.com>,
        "Kalderon, Michal" <Michal.Kalderon@...ium.com>,
        Ariel Elior <Ariel.Elior@...ium.com>,
        "David S. Miller" <davem@...emloft.net>,
        Bart Van Assche <bart.vanassche@...disk.com>,
        Parav Pandit <parav@...lanox.com>,
        Noa Osherovich <noaos@...lanox.com>,
        linux-rdma <linux-rdma@...r.kernel.org>,
        Linux Kernel Mailinglist <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] infiniband: avoid overflow warning

On Mon, 2017-07-31 at 14:18 -0700, Kees Cook wrote:
> On Mon, Jul 31, 2017 at 2:10 PM, Arnd Bergmann <arnd@...db.de> wrote:
> > On Mon, Jul 31, 2017 at 10:58 PM, Kees Cook <keescook@...omium.org>
> > wrote:
> > > On Mon, Jul 31, 2017 at 12:30 AM, Arnd Bergmann <arnd@...db.de>
> > > wrote:
> > > > On Mon, Jul 31, 2017 at 9:08 AM, Moni Shoua <monis@...lanox.com>
> > > > wrote:
> > > > > >                 break;
> > > > > >         default:
> > > > > >                 return -EINVAL;
> > > > > 
> > > > > what happens if you replace 16 with sizeof(struct in6_addr)?
> > > > 
> > > > Same thing: the problem is that gcc already knows the size of
> > > > the structure we
> > > > pass in here, and it is in fact shorter.
> > > 
> > > So gcc is ignoring both the cast (to 16 byte struct in6_addr) and
> > > the
> > > caller's actual 128 byte struct sockaddr_storage, and looking only
> > > at
> > > struct sockaddr? That seems really weird.
> > 
> > Using a sockaddr_storage on the stack would address the warning, but
> > the question was about just changing the hardcoded 16 to a sizeof()
> > operation, and that has no effect.
> 
> Right, I didn't mean that; I was curious why the fortify macro
> resulted in an error at all. The callers are casting from struct
> sockaddr_storage (large enough) to struct sockaddr (not large enough),
> and then the inline is casting back to sockaddr_in6 (large enough). I
> would have expected fortify to check either sockaddr_storage or
> sockaddr_in6, but not sockaddr.
> 
> -Kees
> 

I don't think that's quite what's happening. It will report unknown if
it can't find allocation sites or other guarantees of size. There are no
alloc_size markers yet so it *mostly* does stack / global checks. It
won't infer sizes based on pointer types. It's not a heuristic.

Hoowever, fortify / -fsanitize=object-size can indirectly uncover other
forms of undefined behavior though. Code may rely on doing something
undefined that GCC actively assumes can't happen for optimization. It
can have false positives due to dead code with the compile-time errors
but if the code is actually called with the size that it rejects as
invalid, then it's unlikely there isn't a bug.