netdev - Re: [PATCH v2] net: usb: rtl8150: prevent set_ethernet

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 2 Oct 2020 17:35:25 +0530
From:   Anant Thazhemadam <anant.thazhemadam@...il.com>
To:     Greg KH <greg@...ah.com>
Cc:     David Miller <davem@...emloft.net>,
        linux-kernel-mentees@...ts.linuxfoundation.org,
        syzbot+abbc768b560c84d92fd3@...kaller.appspotmail.com,
        petkan@...leusys.com, kuba@...nel.org, linux-usb@...r.kernel.org,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] net: usb: rtl8150: prevent set_ethernet_addr from
 setting uninit address


On 02-10-2020 17:24, Greg KH wrote:
> On Fri, Oct 02, 2020 at 05:04:13PM +0530, Anant Thazhemadam wrote:
>> On 02/10/20 7:45 am, David Miller wrote:
>>> From: Anant Thazhemadam <anant.thazhemadam@...il.com>
>>> Date: Thu,  1 Oct 2020 13:02:20 +0530
>>>
>>>> When get_registers() fails (which happens when usb_control_msg() fails)
>>>> in set_ethernet_addr(), the uninitialized value of node_id gets copied
>>>> as the address.
>>>>
>>>> Checking for the return values appropriately, and handling the case
>>>> wherein set_ethernet_addr() fails like this, helps in avoiding the
>>>> mac address being incorrectly set in this manner.
>>>>
>>>> Reported-by: syzbot+abbc768b560c84d92fd3@...kaller.appspotmail.com
>>>> Tested-by: syzbot+abbc768b560c84d92fd3@...kaller.appspotmail.com
>>>> Signed-off-by: Anant Thazhemadam <anant.thazhemadam@...il.com>
>>>> Acked-by: Petko Manolov <petkan@...leusys.com>
>>> First, please remove "Linux-kernel-mentees" from the Subject line.
>>>
>>> All patch submitters should have their work judged equally, whoever
>>> they are.  So this Subject text gives no extra information, and it
>>> simply makes scanning Subject lines in one's mailer more difficult.
>> I will keep that in mind for all future submissions. Thank you.
>>
>>> Second, when a MAC address fails to probe a random MAC address should
>>> be selected.  We have helpers for this.  This way an interface still
>>> comes up and is usable, even in the event of a failed MAC address
>>> probe.
>> Okay... I see.
>> But this patch is about ensuring that an uninitialized variable's
>> value (whatever that may be) is not set as the ethernet address
>> blindly (without any form of checking if get_registers() worked
>> as expected, or not). And I didn't think uninitialized values being
>> set as MAC address was considered a good outcome (after all, it
>> seemed to have triggered a bug), especially when it could have
>> been avoided by introducing a simple check that doesn't break
>> anything.
> If the read from the device for the MAC address fails, don't abort the
> whole probe process and make the device not work at all, call the
> networking core to assign a random MAC address.
>
>> However, if I was mistaken, and if that is something that we can live
>> with after all, then I don't really see the understand the purpose of
>> similar checks being made (in all the many places that the return
>> value of get_registers() (or a similar function gets checked) in the first
>> place at all.
> Different values and registers determine what should be done with an
> error.  It's all relative.
>
> For this type of error, we should gracefully recover and keep on going.
> For others, maybe we just ignore the issue, or log it, or something
> else, it all depends.
>
> hope this helps,
>
> greg k-h
Yes, this clears things up for me. I'll see to it that this gets done in a v3.

Thanks,
Anant