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@xxxxxxxxx> >>> 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@xxxxxxxxxxxxxxxxxxxxxxxxx >>>> Tested-by: syzbot+abbc768b560c84d92fd3@xxxxxxxxxxxxxxxxxxxxxxxxx >>>> Signed-off-by: Anant Thazhemadam <anant.thazhemadam@xxxxxxxxx> >>>> Acked-by: Petko Manolov <petkan@xxxxxxxxxxxxx> >>> 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
