linux-kernel - Re: [PATCH v8 15/19] staging: r8188eu: change the type of a variable in rtw

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: <20210920131036.GR2088@kadam>
Date:   Mon, 20 Sep 2021 16:10:36 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     "Fabio M. De Francesco" <fmdefrancesco@...il.com>
Cc:     Larry Finger <Larry.Finger@...inger.net>,
        Phillip Potter <phil@...lpotter.co.uk>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Pavel Skripkin <paskripkin@...il.com>,
        linux-staging@...ts.linux.dev, linux-kernel@...r.kernel.org,
        David Laight <david.Laight@...lab.com>,
        Martin Kaiser <martin@...ser.cx>
Subject: Re: [PATCH v8 15/19] staging: r8188eu: change the type of a variable
 in rtw_read16()

On Mon, Sep 20, 2021 at 03:03:44PM +0200, Fabio M. De Francesco wrote:
> On Monday, September 20, 2021 1:56:47 PM CEST Dan Carpenter wrote:
> > On Mon, Sep 20, 2021 at 01:53:52AM +0200, Fabio M. De Francesco wrote:
> > > Change the type of "data" from __le32 to __le16.
> > > 
> > 
> > You should note in the commit message that:
> > 
> > The last two bytes of "data" are not initialized so the le32_to_cpu(data)
> > technically reads uninitialized data.  This can likely be detected by
> > the KASan checker as reading uninitialized data.  But because the bytes
> > are discarded in the end so this will not affect runtime.
> > 
> > regards,
> > dan carpenter
> > 
> 
> Dear Dan,
> 
> Thanks for your suggestion about this specific topic. 
> 
> We thought that, since "data" is in bitwise AND with 0xffff before being 
> passed to the callee, it was enough to have reviewers know why we're doing 
> that change of type with no further explanations. Actually it seems to be not 
> enough to motivate that change.
> 
> We will surely use the note you provided. 
> 
> However, since I'm not used to blindly follow suggestions (even if I trust 
> your words with no doubts at all) without complete understanding of what I'm 
> doing, I will need to understand what KASan is before copy-paste your note.

Google is your friend!

Either way reading uninitialized data is generally bad.  The trickier
thing is showing that your changes don't affect runtime.  For both of
these le32 to le16 changes.

regards,
dan carpenter