Re: [PATCH] staging: vme_user: fix heap OOB in buffer_from_user and buffer_to_user

[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@xxxxxxxxxxxxxxx and linux-kernel@xxxxxxxxxxxxxxx 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