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

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

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