Linux kernel -stable discussions
 help / color / mirror / Atom feed
* [PATCH] staging: vme_user: fix heap OOB in buffer_from_user and buffer_to_user
@ 2026-06-14 19:53 Michael Tautschnig
  2026-06-15  2:27 ` Greg Kroah-Hartman
  2026-06-18 11:47 ` [PATCH v2] staging: vme_user: bound slave read/write to the kern_buf size Michael Tautschnig
  0 siblings, 2 replies; 4+ messages in thread
From: Michael Tautschnig @ 2026-06-14 19:53 UTC (permalink / raw)
  To: Greg Kroah-Hartman; +Cc: Michael Tautschnig, stable

The SLAVE-path read/write helpers buffer_to_user() and
buffer_from_user() copy 'count' bytes into/out of the fixed-size
kern_buf (PCI_BUF_SIZE = 0x20000 = 128 KiB) without bounding
count against size_buf.

The caller vme_user_write()/vme_user_read() only clamps count to
the VME window size (image_size = vme_get_size(resource)), which
VME_SET_SLAVE sets from user-supplied slave.size — validated
against the VME address space (up to VME_A32_MAX = 4 GiB), NOT
against PCI_BUF_SIZE.  When the window exceeds 128 KiB, a
write()/read() copies past the kern_buf allocation.

Fix by clamping count against size_buf in both buffer_from_user()
and buffer_to_user(), with an early return when *ppos >= size_buf.
This mirrors the existing bounds check in resource_from_user() (the
MASTER-path helper).

The bug was found by static analysis (CodeQL taint tracking + CBMC
bounded model checking) and dynamically confirmed under KASAN with
the vme_fake bridge:

  BUG: KASAN: slab-out-of-bounds in _copy_from_user+0x2d/0x80
  Write of size 262144 at addr ffff888004100000 by task trigger/68
    _copy_from_user+0x2d/0x80
    vme_user_write+0x13e/0x240 [vme_user]
    vfs_write+0x1b8/0x7a0
    ksys_write+0xb8/0x150

Cc: stable@vger.kernel.org
Signed-off-by: Michael Tautschnig <tautschn@amazon.com>
---
 drivers/staging/vme_user/vme_user.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/staging/vme_user/vme_user.c b/drivers/staging/vme_user/vme_user.c
index 5829a4141..bacf6a7d6 100644
--- a/drivers/staging/vme_user/vme_user.c
+++ b/drivers/staging/vme_user/vme_user.c
@@ -156,6 +156,15 @@ static ssize_t buffer_to_user(unsigned int minor, char __user *buf,
 {
 	void *image_ptr;
 
+	/* Clamp to the fixed kern_buf (size_buf): the VME window
+	 * (image_size) may exceed PCI_BUF_SIZE, so *ppos + count can
+	 * run past kern_buf otherwise.
+	 */
+	if (*ppos >= image[minor].size_buf)
+		return 0;
+	if (count > image[minor].size_buf - *ppos)
+		count = image[minor].size_buf - *ppos;
+
 	image_ptr = image[minor].kern_buf + *ppos;
 	if (copy_to_user(buf, image_ptr, (unsigned long)count))
 		return -EFAULT;
@@ -168,6 +177,15 @@ static ssize_t buffer_from_user(unsigned int minor, const char __user *buf,
 {
 	void *image_ptr;
 
+	/* Clamp to the fixed kern_buf (size_buf): the VME window
+	 * (image_size) may exceed PCI_BUF_SIZE, so *ppos + count can
+	 * run past kern_buf otherwise.
+	 */
+	if (*ppos >= image[minor].size_buf)
+		return 0;
+	if (count > image[minor].size_buf - *ppos)
+		count = image[minor].size_buf - *ppos;
+
 	image_ptr = image[minor].kern_buf + *ppos;
 	if (copy_from_user(image_ptr, buf, (unsigned long)count))
 		return -EFAULT;
-- 
2.43.0


Amazon Development Center Austria GmbH
Brueckenkopfgasse 1
8020 Graz
Oesterreich
Sitz in Graz
Firmenbuchnummer: FN 439453 f
Firmenbuchgericht: Landesgericht fuer Zivilrechtssachen Graz



^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH] staging: vme_user: fix heap OOB in buffer_from_user and buffer_to_user
  2026-06-14 19:53 [PATCH] staging: vme_user: fix heap OOB in buffer_from_user and buffer_to_user Michael Tautschnig
@ 2026-06-15  2:27 ` Greg Kroah-Hartman
  2026-06-18 11:46   ` Michael Tautschnig
  2026-06-18 11:47 ` [PATCH v2] staging: vme_user: bound slave read/write to the kern_buf size Michael Tautschnig
  1 sibling, 1 reply; 4+ messages in thread
From: Greg Kroah-Hartman @ 2026-06-15  2:27 UTC (permalink / raw)
  To: Michael Tautschnig; +Cc: stable

On Sun, Jun 14, 2026 at 09:53:08PM +0200, Michael Tautschnig wrote:
> The SLAVE-path read/write helpers buffer_to_user() and
> buffer_from_user() copy 'count' bytes into/out of the fixed-size
> kern_buf (PCI_BUF_SIZE = 0x20000 = 128 KiB) without bounding
> count against size_buf.
> 
> The caller vme_user_write()/vme_user_read() only clamps count to
> the VME window size (image_size = vme_get_size(resource)), which
> VME_SET_SLAVE sets from user-supplied slave.size — validated
> against the VME address space (up to VME_A32_MAX = 4 GiB), NOT
> against PCI_BUF_SIZE.  When the window exceeds 128 KiB, a
> write()/read() copies past the kern_buf allocation.
> 
> Fix by clamping count against size_buf in both buffer_from_user()
> and buffer_to_user(), with an early return when *ppos >= size_buf.
> This mirrors the existing bounds check in resource_from_user() (the
> MASTER-path helper).
> 
> The bug was found by static analysis (CodeQL taint tracking + CBMC
> bounded model checking) and dynamically confirmed under KASAN with
> the vme_fake bridge:
> 
>   BUG: KASAN: slab-out-of-bounds in _copy_from_user+0x2d/0x80
>   Write of size 262144 at addr ffff888004100000 by task trigger/68
>     _copy_from_user+0x2d/0x80
>     vme_user_write+0x13e/0x240 [vme_user]
>     vfs_write+0x1b8/0x7a0
>     ksys_write+0xb8/0x150
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Michael Tautschnig <tautschn@amazon.com>
> ---
>  drivers/staging/vme_user/vme_user.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)

Nice, but why did you not use get_maintainers.pl to determine who to
send this to?  You cut out the staging list?


> 
> diff --git a/drivers/staging/vme_user/vme_user.c b/drivers/staging/vme_user/vme_user.c
> index 5829a4141..bacf6a7d6 100644
> --- a/drivers/staging/vme_user/vme_user.c
> +++ b/drivers/staging/vme_user/vme_user.c
> @@ -156,6 +156,15 @@ static ssize_t buffer_to_user(unsigned int minor, char __user *buf,
>  {
>  	void *image_ptr;
>  
> +	/* Clamp to the fixed kern_buf (size_buf): the VME window
> +	 * (image_size) may exceed PCI_BUF_SIZE, so *ppos + count can
> +	 * run past kern_buf otherwise.
> +	 */

Not a network driver, so you can use normal coding style for comments.

> +	if (*ppos >= image[minor].size_buf)
> +		return 0;

No error?

> +	if (count > image[minor].size_buf - *ppos)
> +		count = image[minor].size_buf - *ppos;

Why are you covering up for userspace errors, shouldn't this just fail?

> +
>  	image_ptr = image[minor].kern_buf + *ppos;
>  	if (copy_to_user(buf, image_ptr, (unsigned long)count))
>  		return -EFAULT;
> @@ -168,6 +177,15 @@ static ssize_t buffer_from_user(unsigned int minor, const char __user *buf,
>  {
>  	void *image_ptr;
>  
> +	/* Clamp to the fixed kern_buf (size_buf): the VME window
> +	 * (image_size) may exceed PCI_BUF_SIZE, so *ppos + count can
> +	 * run past kern_buf otherwise.
> +	 */
> +	if (*ppos >= image[minor].size_buf)
> +		return 0;

Again, why not return an error?

> +	if (count > image[minor].size_buf - *ppos)
> +		count = image[minor].size_buf - *ppos;

Same here, shouldn't this fail?

And no chance for this to wrap again, right?

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] staging: vme_user: fix heap OOB in buffer_from_user and buffer_to_user
  2026-06-15  2:27 ` Greg Kroah-Hartman
@ 2026-06-18 11:46   ` Michael Tautschnig
  0 siblings, 0 replies; 4+ messages in thread
From: Michael Tautschnig @ 2026-06-18 11:46 UTC (permalink / raw)
  To: gregkh; +Cc: linux-staging, linux-kernel, stable, Michael Tautschnig

On Mon, Jun 15, 2026 at 04:27:38AM +0200, Greg Kroah-Hartman wrote:
> Nice, but why did you not use get_maintainers.pl to determine who to
> send this to?  You cut out the staging list?

My fault -- v1 went only to you and stable@.  v2 is Cc'd to
linux-staging@lists.linux.dev and linux-kernel@vger.kernel.org per
scripts/get_maintainer.pl (VME is S: Orphan, so that's the full set).

> > +	/* Clamp to the fixed kern_buf (size_buf): the VME window
> > +	 * (image_size) may exceed PCI_BUF_SIZE, so *ppos + count can
> > +	 * run past kern_buf otherwise.
> > +	 */
>
> Not a network driver, so you can use normal coding style for comments.

Fixed in v2 -- switched to the normal (non-networking) multi-line comment
style.

> > +	if (*ppos >= image[minor].size_buf)
> > +		return 0;
>
> No error?
[...]
> Again, why not return an error?

This is the end-of-buffer case, and returning 0 matches what the driver
already does: vme_user_read()/vme_user_write() already "return 0" when
*ppos > image_size - 1.  A 0-byte return at/after the end of the buffer
is the normal read(2)/write(2) convention (EOF for read; for write it
mirrors the driver's existing behaviour).  Returning an error here would
make the SLAVE path diverge from both the existing *ppos check two lines
up and the MASTER path, which seemed wrong for a Cc: stable fix.

> > +	if (count > image[minor].size_buf - *ppos)
> > +		count = image[minor].size_buf - *ppos;
>
> Why are you covering up for userspace errors, shouldn't this just fail?
[...]
> Same here, shouldn't this fail?

The clamp mirrors behaviour that already exists in this driver rather
than inventing new policy: vme_user_read()/vme_user_write() already clamp
count to image_size - *ppos (a short transfer), and the MASTER-path
resource_to_user()/resource_from_user() already clamp count to size_buf.
A short transfer is standard read/write semantics -- userspace is
expected to loop -- so the SLAVE path was simply missing the size_buf
bound that MASTER already has; it clamped to the VME window (image_size,
up to 4 GiB from the user-set slave.size) instead of the fixed 128 KiB
kern_buf.  v2 keeps the established short-transfer contract and just
fixes the bound.

That said, you're the maintainer: if you'd rather an over-large request
fail outright (e.g. -EFBIG / -ENOSPC) I'm happy to send that, but I'd do
it as a separate, clearly-marked behaviour change rather than fold it
into the stable fix.

> And no chance for this to wrap again, right?

No wrap in the added code: the caller already rejects *ppos < 0, and the
early "if (*ppos >= size_buf) return 0;" means 0 <= *ppos < size_buf at
the subtraction, so "size_buf - *ppos" is a positive u64 and the
"count > size_buf - *ppos" comparison can't overflow.

It also tightens a pre-existing wrap: the caller's
"if (*ppos + count > image_size)" adds loff_t + size_t and can overflow
for a huge count; the new "count > size_buf - *ppos" re-bounds count
regardless, so the copy stays within kern_buf even if that earlier clamp
is bypassed.

v2 also adds Fixes: f00a86d98a1e ("Staging: vme: add VME userspace
driver") and keeps the KASAN reproducer in the changelog.

Thanks,
Michael


Amazon Development Center Austria GmbH
Brueckenkopfgasse 1
8020 Graz
Oesterreich
Sitz in Graz
Firmenbuchnummer: FN 439453 f
Firmenbuchgericht: Landesgericht fuer Zivilrechtssachen Graz




^ permalink raw reply	[flat|nested] 4+ messages in thread

* [PATCH v2] staging: vme_user: bound slave read/write to the kern_buf size
  2026-06-14 19:53 [PATCH] staging: vme_user: fix heap OOB in buffer_from_user and buffer_to_user Michael Tautschnig
  2026-06-15  2:27 ` Greg Kroah-Hartman
@ 2026-06-18 11:47 ` Michael Tautschnig
  1 sibling, 0 replies; 4+ messages in thread
From: Michael Tautschnig @ 2026-06-18 11:47 UTC (permalink / raw)
  To: gregkh; +Cc: linux-staging, linux-kernel, stable, Michael Tautschnig

The SLAVE-path helpers buffer_to_user() and buffer_from_user() copy
'count' bytes into/out of the fixed-size kern_buf (size_buf ==
PCI_BUF_SIZE == 0x20000, 128 KiB) using *ppos as the offset, without
bounding *ppos + count against size_buf.

vme_user_write()/vme_user_read() only clamp count to the VME window size
(image_size = vme_get_size(resource)), which VME_SET_SLAVE sets from the
user-supplied slave.size -- validated against the VME address space (up
to VME_A32_MAX = 4 GiB), not against PCI_BUF_SIZE.  When the window
exceeds 128 KiB, a write()/read() copies past the kern_buf allocation.

Clamp count against size_buf in both helpers, with an early return when
*ppos is already at/after the buffer end.  *ppos is >= 0 here (the caller
rejects negative offsets), so size_buf - *ppos cannot wrap.  This mirrors
the existing clamp in the MASTER-path helpers resource_to_user() /
resource_from_user(), and matches the read()/write() convention of a
short transfer at end-of-buffer.

Found by static analysis (CodeQL taint tracking + CBMC bounded model
checking) and confirmed dynamically under KASAN with the vme_fake bridge:

  BUG: KASAN: slab-out-of-bounds in _copy_from_user+0x2d/0x80
  Write of size 262144 at addr ffff888004100000 by task trigger/68
    _copy_from_user+0x2d/0x80
    vme_user_write+0x13e/0x240 [vme_user]
    vfs_write+0x1b8/0x7a0
    ksys_write+0xb8/0x150

Fixes: f00a86d98a1e ("Staging: vme: add VME userspace driver")
Cc: stable@vger.kernel.org
Signed-off-by: Michael Tautschnig <tautschn@amazon.com>
---
v1 -> v2:
 - Cc the staging list and LKML per get_maintainer.pl (v1 reached only
   Greg KH).
 - Switch the added comment to normal (non-networking) kernel style.
 - Keep the clamp + early return: this matches the read()/write()
   short-transfer / EOF convention already used by vme_user_read() /
   vme_user_write() (which clamp count and return 0 past the window) and
   by the MASTER-path resource_*() helpers (which clamp count to
   size_buf).  Documented that *ppos >= 0 here, so the subtraction is
   wrap-free.

Link to v1: https://lore.kernel.org/all/20260614195318.40397-1-tautschn@amazon.com/

 drivers/staging/vme_user/vme_user.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/staging/vme_user/vme_user.c b/drivers/staging/vme_user/vme_user.c
index 11e25c2f6b0a..a472a38ef613 100644
--- a/drivers/staging/vme_user/vme_user.c
+++ b/drivers/staging/vme_user/vme_user.c
@@ -156,6 +156,17 @@ static ssize_t buffer_to_user(unsigned int minor, char __user *buf,
 {
 	void *image_ptr;
 
+	/*
+	 * The slave window (image_size) can exceed the fixed kern_buf
+	 * (size_buf == PCI_BUF_SIZE), so bound the copy to kern_buf.
+	 * *ppos is >= 0 here (checked by the caller), so the
+	 * subtraction below cannot wrap.
+	 */
+	if (*ppos >= image[minor].size_buf)
+		return 0;
+	if (count > image[minor].size_buf - *ppos)
+		count = image[minor].size_buf - *ppos;
+
 	image_ptr = image[minor].kern_buf + *ppos;
 	if (copy_to_user(buf, image_ptr, (unsigned long)count))
 		return -EFAULT;
@@ -168,6 +179,17 @@ static ssize_t buffer_from_user(unsigned int minor, const char __user *buf,
 {
 	void *image_ptr;
 
+	/*
+	 * The slave window (image_size) can exceed the fixed kern_buf
+	 * (size_buf == PCI_BUF_SIZE), so bound the copy to kern_buf.
+	 * *ppos is >= 0 here (checked by the caller), so the
+	 * subtraction below cannot wrap.
+	 */
+	if (*ppos >= image[minor].size_buf)
+		return 0;
+	if (count > image[minor].size_buf - *ppos)
+		count = image[minor].size_buf - *ppos;
+
 	image_ptr = image[minor].kern_buf + *ppos;
 	if (copy_from_user(image_ptr, buf, (unsigned long)count))
 		return -EFAULT;
-- 
2.34.1



Amazon Development Center Austria GmbH
Brueckenkopfgasse 1
8020 Graz
Oesterreich
Sitz in Graz
Firmenbuchnummer: FN 439453 f
Firmenbuchgericht: Landesgericht fuer Zivilrechtssachen Graz




^ permalink raw reply related	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2026-06-18 11:47 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-06-14 19:53 [PATCH] staging: vme_user: fix heap OOB in buffer_from_user and buffer_to_user Michael Tautschnig
2026-06-15  2:27 ` Greg Kroah-Hartman
2026-06-18 11:46   ` Michael Tautschnig
2026-06-18 11:47 ` [PATCH v2] staging: vme_user: bound slave read/write to the kern_buf size Michael Tautschnig

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox