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... Sounds reasonable, testing error paths of "short reads" is something that people are now only starting to notice and work to resolve. Patches to resolve this are always gladly appreciated! thanks, greg k-h
