Use of uninitialized data in special error case of usb storage transport

Use of uninitialized data in special error case of usb storage transport

[Lukas Bulwahn]

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...


Thanks and best regards,

Lukas

Re: Use of uninitialized data in special error case of usb storage transport

[Alan Stern]

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

Re: Use of uninitialized data in special error case of usb storage transport

[Greg Kroah-Hartman]

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