Message-ID: <780e991d-864d-0491-f440-12a926920a8a@gmail.com>
Date:   Wed, 16 Sep 2020 19:08:21 +0530
From:   Anant Thazhemadam <anant.thazhemadam@...il.com>
To:     Petko Manolov <petkan@...leusys.com>
Cc:     linux-kernel-mentees@...ts.linuxfoundation.org,
        syzbot+abbc768b560c84d92fd3@...kaller.appspotmail.com,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>, linux-usb@...r.kernel.org,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [Linux-kernel-mentees][PATCH] rtl8150: set memory to all 0xFFs on
 failed register reads


On 16/09/20 11:49 am, Petko Manolov wrote:
> On 20-09-16 10:35:40, Anant Thazhemadam wrote:
>> get_registers() copies whatever memory is written by the
>> usb_control_msg() call even if the underlying urb call ends up failing.
> Not true, memcpy() is only called if "ret" is positive.
Right. I'm really sorry I fumbled and messed up the commit message
there. Thank you for pointing that out.
>> If get_registers() fails, or ends up reading 0 bytes, meaningless and junk 
>> register values would end up being copied over (and eventually read by the 
>> driver), and since most of the callers of get_registers() don't check the 
>> return values of get_registers() either, this would go unnoticed.
> usb_control_msg() returns negative on error (look up usb_internal_control_msg() 
> to see for yourself) so it does not go unnoticed.

When I said "this would go unnoticed", I meant get_register() failing would
go unnoticed, not that usb_control_msg() failing would go unnoticed.
I agree that get_registers() notices usb_control_msg() failing, and
appropriately returns the return value from usb_control_msg().
But there are many instances where get_registers() is called but the return
value of get_registers() is not checked, to see if it failed or not; hence, "this
would go unnoticed".

> If for some reason it return zero, nothing is copied.  Also, if usb transfer fail 
> no register values are being copied anywhere.

True.
Now consider set_ethernet_addr(), and suppose get_register() fails when
invoked from inside set_ethernet_addr().
As you said, no value is copied back, which means no value is copied back
into node_id, which leaves node_id uninitialized. This node_id (still
uninitialized) is then blindly copied into dev->netdev->dev_addr; which
is less than ideal and could also quickly prove to become an issue, right?

> Your patch also allows for memcpy() to be called with 'size' either zero or 
> greater than the allocated buffer size. Please, look at the code carefully.
Oh. I apologize for this. This can be reverted relatively easily.
>> It might be a better idea to try and mirror the PCI master abort
>> termination and set memory to 0xFFs instead in such cases.
> I wasn't aware drivers are now responsible for filling up the memory with 
> anything.  Does not sound like a good idea to me.
Since we copy the correct register values when get_register() doesn't fail,
I thought it might be a slightly better alternative to fill node_id with 0xFFs,
instead of leaving it go uninitialized in case get_registers() fails.

Also, what are the odds that a successful get_register() call would see
0xFFs being copied?
If that's very real scenario, then I admit this doesn't work at all.

The only other alternative approach I can think of that can handle the
issue I highlighted above, is to introduce checking for get_registers()'s
return values nearly everywhere it gets called.
Would that be a more preferable and welcome approach?

Thank you for your time.

Thanks,
Anant

Powered by blists - more mailing lists