[PATCH] sandbox: usb: Fix out-of-bounds read when fd=-1

[Simon Glass]

Hi Sean,

On Wed, 23 Mar 2022 at 16:24, Sean Anderson <seanga2 at gmail.com> wrote:
>
> sandbox_flash_bulk uses priv->read_len to determine if priv->buff contains
> the response data (such as from SCSI_INQUIRY). However, if priv->fd=-1 in
> handle_read, then priv->read_len is not set even though we are going to
> PHASE_DATA. This causes sandbox_flash_bulk to try and read len bytes from
> priv->buff, which likely goes past the end of the buffer. Fix this by always
> setting priv->read_len even if we aren't going to read anything.
>
> Fixes: f4f715360c ("dm: usb: sandbox: Add an emulator for USB flash devices")
> Signed-off-by: Sean Anderson <seanga2 at gmail.com>
> ---
> Is returning -EIO correct here? Should we return 0 (nothing read)? Or pretend to
> read the whole thing and then let the caller figure it out based on the status?

It looks like returning an error makes sense, but Marek may know more.

>
>  drivers/usb/emul/sandbox_flash.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)

Reviewed-by: Simon Glass <sjg at chromium.org>

>
> diff --git a/drivers/usb/emul/sandbox_flash.c b/drivers/usb/emul/sandbox_flash.c
> index edabc1b3a7..f20c6ad754 100644
> --- a/drivers/usb/emul/sandbox_flash.c
> +++ b/drivers/usb/emul/sandbox_flash.c
> @@ -228,9 +228,9 @@ static void handle_read(struct sandbox_flash_priv *priv, ulong lba,
>                         ulong transfer_len)
>  {
>         debug("%s: lba=%lx, transfer_len=%lx\n", __func__, lba, transfer_len);
> +       priv->read_len = transfer_len;
>         if (priv->fd != -1) {
>                 os_lseek(priv->fd, lba * SANDBOX_FLASH_BLOCK_LEN, OS_SEEK_SET);
> -               priv->read_len = transfer_len;
>                 setup_response(priv, priv->buff,
>                                transfer_len * SANDBOX_FLASH_BLOCK_LEN);
>         } else {
> @@ -336,6 +336,9 @@ static int sandbox_flash_bulk(struct udevice *dev, struct usb_device *udev,
>                         if (priv->read_len) {
>                                 ulong bytes_read;
>
> +                               if (priv->fd == -1)
> +                                       return -EIO;
> +
>                                 bytes_read = os_read(priv->fd, buff, len);
>                                 if (bytes_read != len)
>                                         return -EIO;
> --
> 2.35.1
>

Regards,
Simon