[PATCH v2] usb: gadget: f_mass_storage: Fix potential integer overflow in check_command_size_in_blocks()

[PATCH v2] usb: gadget: f_mass_storage: Fix potential integer overflow in check_command_size_in_blocks()

[pip-izony]

From: Seungjin Bae <eeodqql09@gmail.com>

The `check_command_size_in_blocks()` function calculates the data size
in bytes by left shifting `common->data_size_from_cmnd` by the block
size (`common->curlun->blkbits`). However, it does not validate whether
this shift operation will cause an integer overflow.

Initially, the block size is set up in `fsg_lun_open()` , and the
`common->data_size_from_cmnd` is set up in `do_scsi_command()`. During
initialization, there is no integer overflow check for the interaction
between two variables.

So if a malicious USB host sends a SCSI READ or WRITE command
requesting a large amount of data (`common->data_size_from_cmnd`), the
left shift operation can wrap around. This results in a truncated data
size, which can bypass boundary checks and potentially lead to memory
corruption or out-of-bounds accesses.

Fix this by using the check_shl_overflow() macro to safely perform the
shift and catch any overflows.

Fixes: 144974e7f9e3 ("usb: gadget: mass_storage: support multi-luns with different logic block size")
Signed-off-by: Seungjin Bae <eeodqql09@gmail.com>
---
 v1 -> v2: Removed the cmnd_size check and applied the check_shl_overflow() macro

 drivers/usb/gadget/function/f_mass_storage.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/function/f_mass_storage.c b/drivers/usb/gadget/function/f_mass_storage.c
index 6af96e2b44eb..cb885153a31e 100644
--- a/drivers/usb/gadget/function/f_mass_storage.c
+++ b/drivers/usb/gadget/function/f_mass_storage.c
@@ -180,6 +180,7 @@
 #include <linux/kthread.h>
 #include <linux/sched/signal.h>
 #include <linux/limits.h>
+#include <linux/overflow.h>
 #include <linux/pagemap.h>
 #include <linux/rwsem.h>
 #include <linux/slab.h>
@@ -1853,8 +1854,17 @@ static int check_command_size_in_blocks(struct fsg_common *common,
 		int cmnd_size, enum data_direction data_dir,
 		unsigned int mask, int needs_medium, const char *name)
 {
-	if (common->curlun)
-		common->data_size_from_cmnd <<= common->curlun->blkbits;
+	unsigned int blkbits;
+
+	if (common->curlun) {
+		blkbits = common->curlun->blkbits;
+		if (check_shl_overflow(common->data_size_from_cmnd, blkbits,
+							   &common->data_size_from_cmnd)) {
+			common->phase_error = 1;
+			return -EINVAL;
+		}
+	}
+
 	return check_command(common, cmnd_size, data_dir,
 			mask, needs_medium, name);
 }
-- 
2.43.0

Re: [PATCH v2] usb: gadget: f_mass_storage: Fix potential integer overflow in check_command_size_in_blocks()

[Alan Stern]

On Thu, Feb 26, 2026 at 10:55:58AM -0500, pip-izony wrote:
> From: Seungjin Bae <eeodqql09@gmail.com>
> 
> The `check_command_size_in_blocks()` function calculates the data size
> in bytes by left shifting `common->data_size_from_cmnd` by the block
> size (`common->curlun->blkbits`). However, it does not validate whether
> this shift operation will cause an integer overflow.
> 
> Initially, the block size is set up in `fsg_lun_open()` , and the
> `common->data_size_from_cmnd` is set up in `do_scsi_command()`. During
> initialization, there is no integer overflow check for the interaction
> between two variables.
> 
> So if a malicious USB host sends a SCSI READ or WRITE command
> requesting a large amount of data (`common->data_size_from_cmnd`), the
> left shift operation can wrap around. This results in a truncated data
> size, which can bypass boundary checks and potentially lead to memory
> corruption or out-of-bounds accesses.
> 
> Fix this by using the check_shl_overflow() macro to safely perform the
> shift and catch any overflows.
> 
> Fixes: 144974e7f9e3 ("usb: gadget: mass_storage: support multi-luns with different logic block size")
> Signed-off-by: Seungjin Bae <eeodqql09@gmail.com>
> ---
>  v1 -> v2: Removed the cmnd_size check and applied the check_shl_overflow() macro
> 
>  drivers/usb/gadget/function/f_mass_storage.c | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/gadget/function/f_mass_storage.c b/drivers/usb/gadget/function/f_mass_storage.c
> index 6af96e2b44eb..cb885153a31e 100644
> --- a/drivers/usb/gadget/function/f_mass_storage.c
> +++ b/drivers/usb/gadget/function/f_mass_storage.c
> @@ -180,6 +180,7 @@
>  #include <linux/kthread.h>
>  #include <linux/sched/signal.h>
>  #include <linux/limits.h>
> +#include <linux/overflow.h>
>  #include <linux/pagemap.h>
>  #include <linux/rwsem.h>
>  #include <linux/slab.h>
> @@ -1853,8 +1854,17 @@ static int check_command_size_in_blocks(struct fsg_common *common,
>  		int cmnd_size, enum data_direction data_dir,
>  		unsigned int mask, int needs_medium, const char *name)
>  {
> -	if (common->curlun)
> -		common->data_size_from_cmnd <<= common->curlun->blkbits;
> +	unsigned int blkbits;
> +
> +	if (common->curlun) {
> +		blkbits = common->curlun->blkbits;
> +		if (check_shl_overflow(common->data_size_from_cmnd, blkbits,
> +							   &common->data_size_from_cmnd)) {

It's a little odd to create a local variable for something that's used 
only once -- you could just as easily have put common->curlun->blkbits 
directly as the macro's argument.  But never mind.

The indentation of the continuation line is strange.  This source file 
uses a couple of different conventions for continuation lines, but none 
of them involve adding 5-1/2 extra tab stops.

These are just formal problems.  There's nothing semantically wrong with 
the patch.

Alan Stern

> +			common->phase_error = 1;
> +			return -EINVAL;
> +		}
> +	}
> +
>  	return check_command(common, cmnd_size, data_dir,
>  			mask, needs_medium, name);
>  }
> -- 
> 2.43.0

Re: [PATCH v2] usb: gadget: f_mass_storage: Fix potential integer overflow in check_command_size_in_blocks()

[Kees Cook]

On Thu, Feb 26, 2026 at 10:55:58AM -0500, pip-izony wrote:
> From: Seungjin Bae <eeodqql09@gmail.com>
> 
> The `check_command_size_in_blocks()` function calculates the data size
> in bytes by left shifting `common->data_size_from_cmnd` by the block
> size (`common->curlun->blkbits`). However, it does not validate whether
> this shift operation will cause an integer overflow.
> 
> Initially, the block size is set up in `fsg_lun_open()` , and the
> `common->data_size_from_cmnd` is set up in `do_scsi_command()`. During
> initialization, there is no integer overflow check for the interaction
> between two variables.
> 
> So if a malicious USB host sends a SCSI READ or WRITE command
> requesting a large amount of data (`common->data_size_from_cmnd`), the
> left shift operation can wrap around. This results in a truncated data
> size, which can bypass boundary checks and potentially lead to memory
> corruption or out-of-bounds accesses.
> 
> Fix this by using the check_shl_overflow() macro to safely perform the
> shift and catch any overflows.
> 
> Fixes: 144974e7f9e3 ("usb: gadget: mass_storage: support multi-luns with different logic block size")
> Signed-off-by: Seungjin Bae <eeodqql09@gmail.com>
> ---
>  v1 -> v2: Removed the cmnd_size check and applied the check_shl_overflow() macro
> 
>  drivers/usb/gadget/function/f_mass_storage.c | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/gadget/function/f_mass_storage.c b/drivers/usb/gadget/function/f_mass_storage.c
> index 6af96e2b44eb..cb885153a31e 100644
> --- a/drivers/usb/gadget/function/f_mass_storage.c
> +++ b/drivers/usb/gadget/function/f_mass_storage.c
> @@ -180,6 +180,7 @@
>  #include <linux/kthread.h>
>  #include <linux/sched/signal.h>
>  #include <linux/limits.h>
> +#include <linux/overflow.h>
>  #include <linux/pagemap.h>
>  #include <linux/rwsem.h>
>  #include <linux/slab.h>
> @@ -1853,8 +1854,17 @@ static int check_command_size_in_blocks(struct fsg_common *common,
>  		int cmnd_size, enum data_direction data_dir,
>  		unsigned int mask, int needs_medium, const char *name)
>  {
> -	if (common->curlun)
> -		common->data_size_from_cmnd <<= common->curlun->blkbits;
> +	unsigned int blkbits;
> +
> +	if (common->curlun) {
> +		blkbits = common->curlun->blkbits;
> +		if (check_shl_overflow(common->data_size_from_cmnd, blkbits,
> +							   &common->data_size_from_cmnd)) {
> +			common->phase_error = 1;
> +			return -EINVAL;
> +		}
> +	}

Right, as Alan suggested, I would expect the code style to be:

	if (common->curlen) {
		if (check_shl_overflow(common->data_size_from_cmnd,
				       common->curlun->blkbits,
				       &common->data_size_from_cmnd)) {
			common->phase_error = 1;
			return -EINVAL;
		}
	}

I would have expected scripts/checkpatch.pl to complain about the
indentation. Did you check your patch with that?

-Kees

-- 
Kees Cook