[PATCH] sandbox: usb: Fix out-of-bounds read when fd=-1
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@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?
drivers/usb/emul/sandbox_flash.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
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;
Hi Sean,
On Wed, 23 Mar 2022 at 16:24, Sean Anderson seanga2@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@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@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
Hi Sean,
On Wed, 23 Mar 2022 at 16:24, Sean Anderson seanga2@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@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@chromium.org
Applied to u-boot-dm, thanks!
participants (2)
-
Sean Anderson -
Simon Glass