[PATCH] usb: xhci: fix missing null termination after copy_from_user()

[PATCH] usb: xhci: fix missing null termination after copy_from_user()

[Weigang He]

The buffer 'buf' is filled by copy_from_user() but is not properly
null-terminated before being used with strncmp(). If userspace provides
fewer than 10 bytes, strncmp() may read beyond the copied data into
uninitialized stack memory.

Add explicit null termination after copy_from_user() to ensure the
buffer is always a valid C string before string operations.

Fixes: 87a03802184c ("xhci: debugfs: add debugfs interface to enable compliance mode for a port")
Cc: stable@vger.kernel.org
Signed-off-by: Weigang He <geoffreyhe2@gmail.com>
---
 drivers/usb/host/xhci-debugfs.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/host/xhci-debugfs.c b/drivers/usb/host/xhci-debugfs.c
index c1eb1036ede95..1e2350de77775 100644
--- a/drivers/usb/host/xhci-debugfs.c
+++ b/drivers/usb/host/xhci-debugfs.c
@@ -347,11 +347,13 @@ static ssize_t xhci_port_write(struct file *file,  const char __user *ubuf,
 	struct xhci_port	*port = s->private;
 	struct xhci_hcd		*xhci = hcd_to_xhci(port->rhub->hcd);
 	char                    buf[32];
+	size_t			len = min_t(size_t, sizeof(buf) - 1, count);
 	u32			portsc;
 	unsigned long		flags;
 
-	if (copy_from_user(&buf, ubuf, min_t(size_t, sizeof(buf) - 1, count)))
+	if (copy_from_user(&buf, ubuf, len))
 		return -EFAULT;
+	buf[len] = '\0';
 
 	if (!strncmp(buf, "compliance", 10)) {
 		/* If CTC is clear, compliance is enabled by default */
-- 
2.34.1

Re: [PATCH] usb: xhci: fix missing null termination after copy_from_user()

[Greg KH]

On Sat, Jan 17, 2026 at 09:46:31AM +0000, Weigang He wrote:
> The buffer 'buf' is filled by copy_from_user() but is not properly
> null-terminated before being used with strncmp(). If userspace provides
> fewer than 10 bytes, strncmp() may read beyond the copied data into
> uninitialized stack memory.

But that's fine, it will not match the check, and so it will stop when
told, so no overflow happens anywhere.

> Add explicit null termination after copy_from_user() to ensure the
> buffer is always a valid C string before string operations.

It's ok, and is valid, and this is all debugging code.  This isn't a
real bug, sorry.

> Fixes: 87a03802184c ("xhci: debugfs: add debugfs interface to enable compliance mode for a port")
> Cc: stable@vger.kernel.org

Nope, this doesn't "fix" anything.

How was this bug found?  What tool did you use for this?  How was it tested?

thanks,

greg k-h

Re: [PATCH] usb: xhci: fix missing null termination after copy_from_user()

[Greg KH]

On Sat, Jan 17, 2026 at 10:13:35AM +0000, he geoffrey wrote:
> This bug is found via static analyzer codeql.

As per our documentation, please always state this.  Please read the
documentation and follow that for your future submissions.

Also note that the analyzer is wrong here, please report it and fix it :)

thanks,

greg k-h

Re: [PATCH] usb: xhci: fix missing null termination after copy_from_user()

[David Laight]

On Sat, 17 Jan 2026 10:58:41 +0100
Greg KH <gregkh@linuxfoundation.org> wrote:

> On Sat, Jan 17, 2026 at 09:46:31AM +0000, Weigang He wrote:
> > The buffer 'buf' is filled by copy_from_user() but is not properly
> > null-terminated before being used with strncmp(). If userspace provides
> > fewer than 10 bytes, strncmp() may read beyond the copied data into
> > uninitialized stack memory.  
> 
> But that's fine, it will not match the check, and so it will stop when
> told, so no overflow happens anywhere.

That's not entirely true.
If the user passes "complianc" (without a '\0') and the on-stack buf[9]
happens to be 'e' then the test will succeed rather than fail.

But the only thing that will get upset is KASAN.

More 'interestingly':
- why is it min_t() not min(), everything is size_t.
- why sizeof(buf) - 1, reading into the last byte won't matter.
- why buf[32] not buf[10], even [16] would be plenty for 'future expansion'.

	David

Re: [PATCH] usb: xhci: fix missing null termination after copy_from_user()

[Greg KH]

On Sat, Jan 17, 2026 at 12:06:32PM +0000, David Laight wrote:
> On Sat, 17 Jan 2026 10:58:41 +0100
> Greg KH <gregkh@linuxfoundation.org> wrote:
> 
> > On Sat, Jan 17, 2026 at 09:46:31AM +0000, Weigang He wrote:
> > > The buffer 'buf' is filled by copy_from_user() but is not properly
> > > null-terminated before being used with strncmp(). If userspace provides
> > > fewer than 10 bytes, strncmp() may read beyond the copied data into
> > > uninitialized stack memory.  
> > 
> > But that's fine, it will not match the check, and so it will stop when
> > told, so no overflow happens anywhere.
> 
> That's not entirely true.
> If the user passes "complianc" (without a '\0') and the on-stack buf[9]
> happens to be 'e' then the test will succeed rather than fail.

Ok, fair enough, but you are root doing this so you can do much worse
things to the system than this :)

> But the only thing that will get upset is KASAN.

Agreed.

> More 'interestingly':
> - why is it min_t() not min(), everything is size_t.
> - why sizeof(buf) - 1, reading into the last byte won't matter.
> - why buf[32] not buf[10], even [16] would be plenty for 'future expansion'.

It's debugfs, who are we to judge :)

greg k-h