Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [Linux-kernel-mentees][PATCH] rtl8150: set memory to all 0xFFs on
 failed register reads
To:     Petko Manolov <petkan@nucleusys.com>
Cc:     linux-kernel-mentees@lists.linuxfoundation.org,
        syzbot+abbc768b560c84d92fd3@syzkaller.appspotmail.com,
        "David S. Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>, linux-usb@vger.kernel.org,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20200916050540.15290-1-anant.thazhemadam@gmail.com>
 <20200916061946.GA38262@p310>
From:   Anant Thazhemadam <anant.thazhemadam@gmail.com>
Message-ID: <780e991d-864d-0491-f440-12a926920a8a@gmail.com>
Date:   Wed, 16 Sep 2020 19:08:21 +0530
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.10.0
MIME-Version: 1.0
In-Reply-To: <20200916061946.GA38262@p310>
Content-Type: text/plain; charset=windows-1252
Content-Transfer-Encoding: 7bit
Content-Language: en-US
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


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