linux-kernel - Re: [PATCH 1/2] usbip: Fix seqnum sign extension issue in vhci_tx

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAJ=gGT3pAnKUCve1OV5SXVArz6HUdizu9wjM5tPJ_CrQ=sF=nw@mail.gmail.com>
Date: Fri, 3 Jan 2025 23:18:01 +0800
From: xndcn <xndchn@...il.com>
To: Shuah Khan <skhan@...uxfoundation.org>
Cc: Valentina Manea <valentina.manea.m@...il.com>, Shuah Khan <shuah@...nel.org>, 
	Hongren Zheng <i@...ithal.me>, Greg Kroah-Hartman <gregkh@...uxfoundation.org>, 
	"open list:USB OVER IP DRIVER" <linux-usb@...r.kernel.org>, open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/2] usbip: Fix seqnum sign extension issue in vhci_tx_urb

Thanks.

> How did you find the problem?
> Why does it make sense to cast it to u32?

After running with usbip enough time, I happened to see logs like this:
> [ 293.863125] vhci_hcd vhci_hcd.0: Device attached
> [ 294.081110] usb 1-1: new high-speed USB device number 2 using vhci_hcd
> [ 294.193163] usb 1-1: SetAddress Request (2) to port 0
> [ 294.204334] vhci_hcd: cannot find a urb of seqnum 2147483648 max seqnum -2147483648
> [ 294.204850] vhci_hcd: stop threads
> [ 294.204851] vhci_hcd: release socket
> [ 294.204853] vhci_hcd: disconnect device

Then I notice that on 64bit platform, when
atomic_inc_return(&vhci_hcd->seqnum) returns (2147483647 + 1, or
0x80000000),
priv->seqnum (which is unsigned long, i.e. u64 on 64bit) will be
extends to 0xffffffff80000000
So we can fix the issue by cast it to u32.

On Fri, Jan 3, 2025 at 6:06 AM Shuah Khan <skhan@...uxfoundation.org> wrote:
>
> On 12/31/24 09:15, Xiong Nandi wrote:
> > The atomic_inc_return function returns an int, while priv->seqnum is an
> > unsigned long. So we must cast the result to u32 to prevent potential
> > sign extension and size mismatch issues.
> >
>
> How did you find the problem?
> > Signed-off-by: Xiong Nandi <xndchn@...il.com>
> > ---
> >   drivers/usb/usbip/vhci_hcd.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c
> > index b03e5021c25b..f3f260e01791 100644
> > --- a/drivers/usb/usbip/vhci_hcd.c
> > +++ b/drivers/usb/usbip/vhci_hcd.c
> > @@ -675,7 +675,7 @@ static void vhci_tx_urb(struct urb *urb, struct vhci_device *vdev)
> >
> >       spin_lock_irqsave(&vdev->priv_lock, flags);
> >
> > -     priv->seqnum = atomic_inc_return(&vhci_hcd->seqnum);
> > +     priv->seqnum = (u32)atomic_inc_return(&vhci_hcd->seqnum);
>
> Why does it make sense to cast it to u32?
>
> >       if (priv->seqnum == 0xffff)
> >               dev_info(&urb->dev->dev, "seqnum max\n");
>
>
> thanks,
> -- Shuah