linux-kernel - Re: [PATCH] misc: sgi-xp: Properly initialize buf in xpc_get_rsvd_page

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]

Date:   Fri, 24 May 2019 18:00:15 +0200
From:   Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To:     Arnd Bergmann <arnd@...db.de>
Cc:     Nick Desaulniers <ndesaulniers@...gle.com>,
        Nathan Chancellor <natechancellor@...il.com>,
        Cliff Whickman <cpw@....com>,
        Robin Holt <robinmholt@...il.com>,
        LKML <linux-kernel@...r.kernel.org>,
        clang-built-linux <clang-built-linux@...glegroups.com>,
        Stephen Hines <srhines@...gle.com>,
        Tony Luck <tony.luck@...el.com>, rja@....com,
        Fenghua Yu <fenghua.yu@...el.com>
Subject: Re: [PATCH] misc: sgi-xp: Properly initialize buf in
 xpc_get_rsvd_page_pa

On Fri, May 24, 2019 at 09:40:47AM +0200, Arnd Bergmann wrote:
> On Thu, May 23, 2019 at 8:05 PM 'Nick Desaulniers' via Clang Built
> Linux <clang-built-linux@...glegroups.com> wrote:
> >
> > On Thu, May 23, 2019 at 9:20 AM Nathan Chancellor
> > <natechancellor@...il.com> wrote:
> > >
> > > Clang warns:
> > >
> > > drivers/misc/sgi-xp/xpc_partition.c:73:14: warning: variable 'buf' is
> > > uninitialized when used within its own initialization [-Wuninitialized]
> > >         void *buf = buf;
> > >               ~~~   ^~~
> > > 1 warning generated.
> > >
> > > Initialize it to NULL, which is more deterministic.
> > >
> > > Fixes: 279290294662 ("[IA64-SGI] cleanup the way XPC locates the reserved page")
> > > Link: https://github.com/ClangBuiltLinux/linux/issues/466
> > > Suggested-by: Stephen Hines <srhines@...gle.com>
> > > Signed-off-by: Nathan Chancellor <natechancellor@...il.com>
> >
> > From https://github.com/ClangBuiltLinux/linux/issues/466#issuecomment-488781917
> > I tried to follow the rabbit hole, but eventually these void* get
> > converted to u64's and passed along to function that I have no idea
> > whether they handle the value `(u64)(void*)0` or not.  Either way,
> > they definitely don't handle uninitialized values/UB.
> >
> > I was going to cc Robin who's already cc'ed, but looks like this code
> > was last touched 7-10 years ago. + Tony and Fenghua for ia64 since
> > sn_partition_reserved_page_pa is defined in
> > arch/ia64/include/asm/sn/sn_sal.h.
> >
> > In absence of consensus, I'll prefer NULL to uninitialized.
> > Reviewed-by: Nick Desaulniers <ndesaulniers@...gle.com>
> > Thanks Nathan for following up on this.
> 
> I also had to take a look, and I think I understand what's going on,
> and interestingly, the code is correct, both before and after your patch.
> It's described in this comment:
> 
> /*
>  * Returns the physical address of the partition's reserved page through
>  * an iterative number of calls.
>  *
>  * On first call, 'cookie' and 'len' should be set to 0, and 'addr'
>  * set to the nasid of the partition whose reserved page's address is
>  * being sought.
>  * On subsequent calls, pass the values, that were passed back on the
>  * previous call.
>  *
>  * While the return status equals SALRET_MORE_PASSES, keep calling
>  * this function after first copying 'len' bytes starting at 'addr'
>  * into 'buf'. Once the return status equals SALRET_OK, 'addr' will
>  * be the physical address of the partition's reserved page. If the
>  * return status equals neither of these, an error as occurred.
>  */
> static inline s64
> sn_partition_reserved_page_pa(u64 buf, u64 *cookie, u64 *addr, u64 *len)
> 
> so *len is set to zero on the first call and tells the bios how many bytes
> are accessible at 'buf', and it does get updated by the BIOS to tell
> us how many bytes it needs, and then we allocate that and try again.
> 
> With that explanation added,
> 
> Reviewed-by: Arnd Bergmann <arnd@...db.de>

Nathan, can you add this to the changelog comment and add the reviewed
by lines and resend it so I can queue it up?

thanks,

greg k-h