Re: [PATCH v2] net: usb: rtl8150: prevent set_ethernet_addr from setting uninit address

From: Anant Thazhemadam <anant.thazhemadam@gmail.com>
To: David Miller <davem@davemloft.net>
Cc: linux-kernel-mentees@lists.linuxfoundation.org,
	syzbot+abbc768b560c84d92fd3@syzkaller.appspotmail.com,
	petkan@nucleusys.com, kuba@kernel.org, linux-usb@vger.kernel.org,
	netdev@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] net: usb: rtl8150: prevent set_ethernet_addr from setting uninit address
Date: Fri, 2 Oct 2020 17:04:13 +0530	[thread overview]
Message-ID: <83804e93-8f59-4d35-ec61-e9b5e6f00323@gmail.com> (raw)
In-Reply-To: <20201001.191522.1749084221364678705.davem@davemloft.net>

On 02/10/20 7:45 am, David Miller wrote:
> From: Anant Thazhemadam <anant.thazhemadam@gmail.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@syzkaller.appspotmail.com
>> Tested-by: syzbot+abbc768b560c84d92fd3@syzkaller.appspotmail.com
>> Signed-off-by: Anant Thazhemadam <anant.thazhemadam@gmail.com>
>> Acked-by: Petko Manolov <petkan@nucleusys.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.
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.

In all honesty, this confused me a little more than it provided clarity,
and I hope someone could help me shift that balance back to clarity
again.

Thank you for your time.

Thanks,
Anant