On Wed, Nov 11, 2020 at 10:08:26PM +0100, Lukas Bulwahn wrote: > Dear Alan, dear Greg, > > > here is a quick report from the static analysis tool clang-analyzer on > ./drivers/usb/storage/transport.c: > > When usb_stor_bulk_transfer_sglist() returns with USB_STOR_XFER_ERROR, it > returns without writing to its parameter *act_len. > > Further, the two callers of usb_stor_bulk_transfer_sglist(): > > usb_stor_bulk_srb() and > usb_stor_bulk_transfer_sg(), > > use the passed variable partial without checking the return value. Hence, > the uninitialized value of partial is then used in the further execution > of those two functions. > > Clang-analyzer detects this potential control and data flow and warns: > > drivers/usb/storage/transport.c:469:40: warning: The right operand of '-' > is a garbage value [clang-analyzer-core.UndefinedBinaryOperatorResult] > scsi_set_resid(srb, scsi_bufflen(srb) - partial); > ^ > > drivers/usb/storage/transport.c:495:15: warning: Assigned value is garbage > or undefined [clang-analyzer-core.uninitialized.Assign] > length_left -= partial; > ^ > > The tool is right; unfortunately, I do not know anything about the > intended function here. What is the further operation of those two > functions supposed to be when USB_STOR_XFER_ERROR is returned from > usb_stor_bulk_transfer_sglist()? Should the passed arguments remain > untouched, so setting *act_len to zero for the error paths would be > a suitable fix to achieve that. > > A quick hint on that point and I can prepare a patch for you to pick up... > > Given that this code is pretty stable for years and probably in wider > use, the overall functionality is probably resilient to having this local > data being filled with arbitrary undefined data in the error case... but > who knows... Yeah. When a transfer error occurs, I believe the *act_len value is ignored by the higher layers. But it won't hurt to set it to a valid number, just in case. For the two early-return paths in usb_stor_bulk_transfer_sglist() the amount of data transferred is 0. So if act_len isn't NULL, setting *act_len to 0 would be fine. Alan Stern
