[PATCH] drivers: Two potential integer overflow in sbp_make_tpg() and usbg_make_tpg()

[PATCH] drivers: Two potential integer overflow in sbp_make_tpg() and usbg_make_tpg()

[Chen Yufeng]

The variable tpgt in sbp_make_tpg() and usbg_make_tpg() is defined as
unsigned long and is assigned to tpgt->tport_tpgt, which is defined as u16.
This may cause an integer overflow when tpgt is greater than USHRT_MAX
(65535). 

My fix is based on the implementation of tcm_qla2xxx_make_tpg() in 
drivers/scsi/qla2xxx/tcm_qla2xxx.c which limits tpgt to USHRT_MAX.

This patch is similar to
commit 59c816c1f24d ("vhost/scsi: potential memory corruption").

Signed-off-by: Chen Yufeng <chenyufeng@iie.ac.cn>
---
 drivers/target/sbp/sbp_target.c     | 2 +-
 drivers/usb/gadget/function/f_tcm.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/target/sbp/sbp_target.c b/drivers/target/sbp/sbp_target.c
index 3b89b5a70331..525d978ce41f 100644
--- a/drivers/target/sbp/sbp_target.c
+++ b/drivers/target/sbp/sbp_target.c
@@ -1966,7 +1966,7 @@ static struct se_portal_group *sbp_make_tpg(struct se_wwn *wwn,
 
 	if (strstr(name, "tpgt_") != name)
 		return ERR_PTR(-EINVAL);
-	if (kstrtoul(name + 5, 10, &tpgt) || tpgt > UINT_MAX)
+	if (kstrtoul(name + 5, 10, &tpgt) || tpgt > USHRT_MAX)
 		return ERR_PTR(-EINVAL);
 
 	if (tport->tpg) {
diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c
index 5a2e1237f85c..5c570d4c87b5 100644
--- a/drivers/usb/gadget/function/f_tcm.c
+++ b/drivers/usb/gadget/function/f_tcm.c
@@ -1648,7 +1648,7 @@ static struct se_portal_group *usbg_make_tpg(struct se_wwn *wwn,
 
 	if (strstr(name, "tpgt_") != name)
 		return ERR_PTR(-EINVAL);
-	if (kstrtoul(name + 5, 0, &tpgt) || tpgt > UINT_MAX)
+	if (kstrtoul(name + 5, 0, &tpgt) || tpgt > USHRT_MAX)
 		return ERR_PTR(-EINVAL);
 	ret = -ENODEV;
 	mutex_lock(&tpg_instances_lock);
-- 
2.34.1

Re: [PATCH] drivers: Two potential integer overflow in sbp_make_tpg() and usbg_make_tpg()

[Greg KH]

On Thu, Apr 10, 2025 at 10:05:49PM +0800, Chen Yufeng wrote:
> The variable tpgt in sbp_make_tpg() and usbg_make_tpg() is defined as
> unsigned long and is assigned to tpgt->tport_tpgt, which is defined as u16.
> This may cause an integer overflow when tpgt is greater than USHRT_MAX
> (65535). 

Can that actually ever happen?

If so, why not just fix up "tpgt" to be u16?

> My fix is based on the implementation of tcm_qla2xxx_make_tpg() in 
> drivers/scsi/qla2xxx/tcm_qla2xxx.c which limits tpgt to USHRT_MAX.

Again, why not restrict the size of the variable to start with?


> 
> This patch is similar to
> commit 59c816c1f24d ("vhost/scsi: potential memory corruption").
> 
> Signed-off-by: Chen Yufeng <chenyufeng@iie.ac.cn>
> ---
>  drivers/target/sbp/sbp_target.c     | 2 +-
>  drivers/usb/gadget/function/f_tcm.c | 2 +-

You have to split this into two different patches as it goes through two
different trees before we could take it.

thanks,

greg k-h

Re: Re: [PATCH] drivers: Two potential integer overflow in sbp_make_tpg() and usbg_make_tpg()

[Chen Yufeng]

> On Thu, Apr 10, 2025 at 10:05:49PM +0800, Chen Yufeng wrote:
> > The variable tpgt in sbp_make_tpg() and usbg_make_tpg() is defined as
> > unsigned long and is assigned to tpgt->tport_tpgt, which is defined as u16.
> > This may cause an integer overflow when tpgt is greater than USHRT_MAX
> > (65535). 
> 
> Can that actually ever happen?

I'm sorry, but I haven't tried to trigger this vulnerability myself.

> If so, why not just fix up "tpgt" to be u16?

It's certainly possible to change "tpgt" to u16, but even with that 
modification, UINT_MAX should still be removed, as this limit 
would become meaningless.

> > My fix is based on the implementation of tcm_qla2xxx_make_tpg() in 
> > drivers/scsi/qla2xxx/tcm_qla2xxx.c which limits tpgt to USHRT_MAX.
> 
> Again, why not restrict the size of the variable to start with?

You are right. directly restricting the type of "tpgt" will be a better 
approach.I will take your suggestion into account in the two upcoming 
separate patches.

> > This patch is similar to
> > commit 59c816c1f24d ("vhost/scsi: potential memory corruption").
> > 
> > Signed-off-by: Chen Yufeng <chenyufeng@iie.ac.cn>
> > ---
> >  drivers/target/sbp/sbp_target.c     | 2 +-
> >  drivers/usb/gadget/function/f_tcm.c | 2 +-
> 
> You have to split this into two different patches as it goes through two
> different trees before we could take it.

Thanks four your reply. I will split this patch later.

--
Thanks, 

Chen Yufeng