Re: [PATCH v8 15/19] staging: r8188eu: change the type of a variable in rtw_read16()

On Monday, September 20, 2021 3:10:36 PM CEST Dan Carpenter wrote:

On Mon, Sep 20, 2021 at 03:03:44PM +0200, Fabio M. De Francesco wrote:

quoted

On Monday, September 20, 2021 1:56:47 PM CEST Dan Carpenter wrote:

quoted

On Mon, Sep 20, 2021 at 01:53:52AM +0200, Fabio M. De Francesco wrote:

quoted

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)

quoted

quoted

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 

quoted

that change of type with no further explanations. Actually it seems to be 

not 

quoted

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 

quoted

your words with no doubts at all) without complete understanding of what 

I'm 

quoted

doing, I will need to understand what KASan is before copy-paste your 

note.

Google is your friend!

Yes, it is :)

I think you were referring to the KernelMemorySanitizer (KMSan), a detector 
of uses of uninitialized memory (but it seems to not be upstream):
https://github.com/google/kmsan

Instead you wrote about the The Kernel Address Sanitizer (KASan) that seems 
to be a dynamic memory error detector designed to find out-of-bound and use-
after-free bugs (this is upstream):
https://www.kernel.org/doc/html/v5.0/dev-tools/kasan.html

Can you please confirm?

Back to the code... uninitialised data is not a problem in the old code, it's 
just bad design. The new code cannot affect runtime, it's just better design. 

There's no change in runtime behaviour because of different protection nets:
Aside from the bitwise AND that truncate that variable two the size of two 
bytes and set the higher bytes to 0, memcpy() inside usbctrl_vendorreq(), the 
new usb_read() and usb_write uses memcpy() with count = size (and size is 
checked also in rtw_writeN()). 

I can't see any bugs. Just bad design, that we fix and possible sanitizer's 
warning, that disappear with our fixes. Am I right?

Thanks,

Fabio

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