[PATCH 0/2] USB: sisusbvga: Fix integer overflow and NULL dereference

[PATCH 0/2] USB: sisusbvga: Fix integer overflow and NULL dereference

[Vasiliy Kovalev]

This series fixes two issues in the sisusbvga driver found by static
analysis and confirmed through testing with USB gadget emulation:

1. Integer overflow in boundary check of sisusb_clear_vram() that can be
   triggered by a compromised USB device reporting inflated VRAM size.

2. NULL pointer dereference in sisusb_read() when userspace passes a NULL
   buffer to read(), causing immediate kernel panic.

Both issues are reproducible with the 'USB Gadget Tests' framework [1].

[1] https://github.com/kovalev0/usb-gadget-tests

Vasiliy Kovalev (2):
  USB: sisusbvga: Fix integer overflow in sisusb_clear_vram
  USB: sisusbvga: Fix NULL pointer dereference in sisusb_read

 drivers/usb/misc/sisusbvga/sisusbvga.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

-- 
2.50.1

[PATCH 1/2] USB: sisusbvga: Fix integer overflow in sisusb_clear_vram

[Vasiliy Kovalev]

The boundary check in sisusb_clear_vram():

    if (address + length > sisusb->vrambase + sisusb->vramsize)
        length = sisusb->vrambase + sisusb->vramsize - address;

is subject to unsigned 32-bit integer overflow. When address is close to
UINT32_MAX and length is non-trivial, their sum wraps around and the
guard evaluates incorrectly, allowing the check to be bypassed.

The overflow condition requires length > UINT32_MAX - address. Since
address belongs to [vrambase; vrambase + vramsize) where vrambase is
0xd0000000, and length comes from userspace via SUCMD_CLRSCR as a 24-bit
value (max 0xFFFFFF), overflow is only reachable when sisusb->vramsize
exceeds 1 GiB. A compromised USB device can return an arbitrary value for
sisusb->vramsize via SR[0x14], making this condition reachable.

Use check_add_overflow() to detect the overflow explicitly and return 1.
This ensures the driver correctly rejects invalid parameters instead of
proceeding with wrapped-around values.

Found by Linux Verification Center (linuxtesting.org) with Svace.
Tested with 'USB Gadget Tests'[1]:

$ TEST=sisusbvga-fops-svace-int-overflow
$ echo $TEST > tests/list.txt && make && sudo ./check.sh

[1] Link: https://github.com/kovalev0/usb-gadget-tests
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: <stable@vger.kernel.org>
Signed-off-by: Vasiliy Kovalev <kovalev@altlinux.org>
---
 drivers/usb/misc/sisusbvga/sisusbvga.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/misc/sisusbvga/sisusbvga.c b/drivers/usb/misc/sisusbvga/sisusbvga.c
index febf34f9f049..89d566d192aa 100644
--- a/drivers/usb/misc/sisusbvga/sisusbvga.c
+++ b/drivers/usb/misc/sisusbvga/sisusbvga.c
@@ -1301,6 +1301,7 @@ static int sisusb_clear_vram(struct sisusb_usb_data *sisusb,
 {
 	int ret, i;
 	ssize_t j;
+	u32 end_addr;
 
 	if (address < sisusb->vrambase)
 		return 1;
@@ -1308,7 +1309,10 @@ static int sisusb_clear_vram(struct sisusb_usb_data *sisusb,
 	if (address >= sisusb->vrambase + sisusb->vramsize)
 		return 1;
 
-	if (address + length > sisusb->vrambase + sisusb->vramsize)
+	if (check_add_overflow(address, (u32)length, &end_addr))
+		return 1;
+
+	if (end_addr > sisusb->vrambase + sisusb->vramsize)
 		length = sisusb->vrambase + sisusb->vramsize - address;
 
 	if (length <= 0)
-- 
2.50.1

[PATCH 2/2] USB: sisusbvga: Fix NULL pointer dereference in sisusb_read

[Vasiliy Kovalev]

sisusb_read() passes the user-supplied buffer pointer as 'userbuffer' to
sisusb_read_mem_bulk() in two branches:

    /* VRAM path */
    errno = sisusb_read_mem_bulk(sisusb, address,
            NULL, count, buffer, &bytes_read);

    /* MMIO path */
    errno = sisusb_read_mem_bulk(sisusb, address,
            NULL, count, buffer, &bytes_read);

If buffer == NULL (e.g. read(fd, NULL, count) from userspace), both calls
reach sisusb_read_mem_bulk() with kernbuffer=NULL and userbuffer=NULL.
The condition:

    if (userbuffer)

evaluates to false, the kernbuffer path is taken, and the subsequent
dereference:

    swap32 = *((u32 *)kernbuffer);

panics the kernel:

  Oops: general protection fault, probably for non-canonical
        address 0xdffffc0000000000: 0000 [#1] SMP KASAN NOPTI
  KASAN: null-ptr-deref in range [0x0000000000000000-0x0000000000000007]
  CPU: 3 UID: 0 PID: 370 Comm: sisusbvga-fops- Not tainted 6.19.0-next-20260217 #1
  RIP: 0010:sisusb_read_mem_bulk.constprop.0 (drivers/usb/misc/sisusbvga/sisusbvga.c:1171)
  Call Trace:
   <TASK>
   __pfx_sisusb_read_mem_bulk.constprop.0 (drivers/usb/misc/sisusbvga/sisusbvga.c:1092)
   sisusb_read (drivers/usb/misc/sisusbvga/sisusbvga.c:2396)
   vfs_read (fs/read_write.c:572)
   ksys_read (fs/read_write.c:718)
   do_syscall_64 (arch/x86/entry/syscall_64.c:94)
   entry_SYSCALL_64_after_hwframe (arch/x86/entry/entry_64.S:130)
   RIP: 0033:0x7f335af3fefc
   </TASK>

Add a NULL check after the existing sanity checks, before the first
branch, to guard both the VRAM and the MMIO paths. Release the mutex
before returning, consistent with the existing -ENODEV path above.

Found by Linux Verification Center (linuxtesting.org) with Svace.
Tested with 'USB Gadget Tests'[1]:

$ TEST=sisusbvga-fops-svace-null-deref
$ echo $TEST > tests/list.txt && make && sudo ./check.sh

[1] Link: https://github.com/kovalev0/usb-gadget-tests
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: <stable@vger.kernel.org>
Signed-off-by: Vasiliy Kovalev <kovalev@altlinux.org>
---
 drivers/usb/misc/sisusbvga/sisusbvga.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/usb/misc/sisusbvga/sisusbvga.c b/drivers/usb/misc/sisusbvga/sisusbvga.c
index 89d566d192aa..e14deb1955d9 100644
--- a/drivers/usb/misc/sisusbvga/sisusbvga.c
+++ b/drivers/usb/misc/sisusbvga/sisusbvga.c
@@ -2319,6 +2319,11 @@ static ssize_t sisusb_read(struct file *file, char __user *buffer,
 		return -ENODEV;
 	}
 
+	if (!buffer) {
+		mutex_unlock(&sisusb->lock);
+		return -EFAULT;
+	}
+
 	if ((*ppos) >= SISUSB_PCI_PSEUDO_IOPORTBASE &&
 			(*ppos) <  SISUSB_PCI_PSEUDO_IOPORTBASE + 128) {
 
-- 
2.50.1

Re: [lvc-project] [PATCH 2/2] USB: sisusbvga: Fix NULL pointer dereference in sisusb_read

[Fedor Pchelkin]

Hi there,

On Wed, 18. Feb 03:55, Vasiliy Kovalev wrote:
> sisusb_read() passes the user-supplied buffer pointer as 'userbuffer' to
> sisusb_read_mem_bulk() in two branches:
> 
>     /* VRAM path */
>     errno = sisusb_read_mem_bulk(sisusb, address,
>             NULL, count, buffer, &bytes_read);
> 
>     /* MMIO path */
>     errno = sisusb_read_mem_bulk(sisusb, address,
>             NULL, count, buffer, &bytes_read);
> 
> If buffer == NULL (e.g. read(fd, NULL, count) from userspace), both calls
> reach sisusb_read_mem_bulk() with kernbuffer=NULL and userbuffer=NULL.
> The condition:
> 
>     if (userbuffer)
> 
> evaluates to false, the kernbuffer path is taken, and the subsequent
> dereference:
> 
>     swap32 = *((u32 *)kernbuffer);
> 
> panics the kernel:
> 
>   Oops: general protection fault, probably for non-canonical
>         address 0xdffffc0000000000: 0000 [#1] SMP KASAN NOPTI
>   KASAN: null-ptr-deref in range [0x0000000000000000-0x0000000000000007]
>   CPU: 3 UID: 0 PID: 370 Comm: sisusbvga-fops- Not tainted 6.19.0-next-20260217 #1
>   RIP: 0010:sisusb_read_mem_bulk.constprop.0 (drivers/usb/misc/sisusbvga/sisusbvga.c:1171)
>   Call Trace:
>    <TASK>
>    __pfx_sisusb_read_mem_bulk.constprop.0 (drivers/usb/misc/sisusbvga/sisusbvga.c:1092)
>    sisusb_read (drivers/usb/misc/sisusbvga/sisusbvga.c:2396)
>    vfs_read (fs/read_write.c:572)
>    ksys_read (fs/read_write.c:718)
>    do_syscall_64 (arch/x86/entry/syscall_64.c:94)
>    entry_SYSCALL_64_after_hwframe (arch/x86/entry/entry_64.S:130)
>    RIP: 0033:0x7f335af3fefc
>    </TASK>

This implies the error might be hiding in sisusb_read_mem_bulk().  Its
API should clarify the valid combinations of kernbuffer and userbuffer.
Just like sisusb_write_mem_bulk() does, see comment for that function:

 * If data is from userland, set "userbuffer" (and clear "kernbuffer"),
 * if data is in kernel space, set "kernbuffer" (and clear "userbuffer");
 * if neither "kernbuffer" nor "userbuffer" are given, it is assumed
 * that the data already is in the transfer buffer "sisusb->obuf[index]".

I guess something like that may be relevant for sisusb_read_mem_bulk()
as well, e.g. use sisusb->ibuf by default if both buffers are NULL.
Though it's only a blind guess.

I'd rather suggest making sisusb_read_mem_bulk() check kernbuffer and
userbuffer itself and return an error if both of them happen to be NULL.
That at least keeps current behavior, too.

> 
> Add a NULL check after the existing sanity checks, before the first
> branch, to guard both the VRAM and the MMIO paths. Release the mutex
> before returning, consistent with the existing -ENODEV path above.
> 
> Found by Linux Verification Center (linuxtesting.org) with Svace.
> Tested with 'USB Gadget Tests'[1]:
> 
> $ TEST=sisusbvga-fops-svace-null-deref
> $ echo $TEST > tests/list.txt && make && sudo ./check.sh
> 
> [1] Link: https://github.com/kovalev0/usb-gadget-tests
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Vasiliy Kovalev <kovalev@altlinux.org>
> ---
>  drivers/usb/misc/sisusbvga/sisusbvga.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/usb/misc/sisusbvga/sisusbvga.c b/drivers/usb/misc/sisusbvga/sisusbvga.c
> index 89d566d192aa..e14deb1955d9 100644
> --- a/drivers/usb/misc/sisusbvga/sisusbvga.c
> +++ b/drivers/usb/misc/sisusbvga/sisusbvga.c
> @@ -2319,6 +2319,11 @@ static ssize_t sisusb_read(struct file *file, char __user *buffer,
>  		return -ENODEV;
>  	}
>  
> +	if (!buffer) {
> +		mutex_unlock(&sisusb->lock);
> +		return -EFAULT;
> +	}

It's possible to perform the check without grabbing the mutex overall.
Another nit: this returns -EFAULT.  I think it's is supposed to be used
when the actual page fault has happened.  Incorrect parameters are usually
denied with -EINVAL.  Anyway, that's not a big deal here and there is no
strict documentation on that part.

Thanks.

> +
>  	if ((*ppos) >= SISUSB_PCI_PSEUDO_IOPORTBASE &&
>  			(*ppos) <  SISUSB_PCI_PSEUDO_IOPORTBASE + 128) {
>  
> -- 
> 2.50.1