Hi Sean, On Wed, 23 Mar 2022 at 16:24, Sean Anderson <[email protected]> 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 <[email protected]> > --- > 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 <[email protected]> > > 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
