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
