[PATCH] staging: vme_user: bound slave windows to DMA buffers

[PATCH] staging: vme_user: bound slave windows to DMA buffers

[Yousef Alhouseen]

vme_user allocates a fixed PCI_BUF_SIZE coherent DMA buffer for each
slave image, but VME_SET_SLAVE passes the user-supplied size to
vme_slave_set() unchanged. An enabled window larger than PCI_BUF_SIZE lets
the bridge expose DMA addresses beyond the allocation.

The character device read/write paths also derive their bounds from
vme_get_size(), so the oversized programmed window can make
buffer_to_user() and buffer_from_user() access past
image[minor].kern_buf.

Reject enabled slave windows that do not fit in the backing buffer and use
the same capped size for slave read/write/llseek bounds. Also convert the
read/write limit checks to subtraction-based form so offset + count cannot
wrap around the image size check.

Signed-off-by: Yousef Alhouseen <alhouseenyousef@gmail.com>
---
 drivers/staging/vme_user/vme_user.c | 38 ++++++++++++++++++++++-------
 1 file changed, 29 insertions(+), 9 deletions(-)

diff --git a/drivers/staging/vme_user/vme_user.c b/drivers/staging/vme_user/vme_user.c
index 11e25c2f6..24d3c6ec3 100644
--- a/drivers/staging/vme_user/vme_user.c
+++ b/drivers/staging/vme_user/vme_user.c
@@ -175,11 +175,22 @@ static ssize_t buffer_from_user(unsigned int minor, const char __user *buf,
 	return count;
 }
 
+static size_t vme_user_get_image_size(unsigned int minor)
+{
+	size_t image_size = vme_get_size(image[minor].resource);
+
+	if (type[minor] == SLAVE_MINOR)
+		image_size = min_t(size_t, image_size, image[minor].size_buf);
+
+	return image_size;
+}
+
 static ssize_t vme_user_read(struct file *file, char __user *buf, size_t count,
 			     loff_t *ppos)
 {
 	unsigned int minor = iminor(file_inode(file));
 	ssize_t retval;
+	size_t offset;
 	size_t image_size;
 
 	if (minor == CONTROL_MINOR)
@@ -188,17 +199,19 @@ static ssize_t vme_user_read(struct file *file, char __user *buf, size_t count,
 	mutex_lock(&image[minor].mutex);
 
 	/* XXX Do we *really* want this helper - we can use vme_*_get ? */
-	image_size = vme_get_size(image[minor].resource);
+	image_size = vme_user_get_image_size(minor);
 
 	/* Ensure we are starting at a valid location */
-	if ((*ppos < 0) || (*ppos > (image_size - 1))) {
+	if ((*ppos < 0) || ((u64)*ppos >= image_size)) {
 		mutex_unlock(&image[minor].mutex);
 		return 0;
 	}
 
+	offset = *ppos;
+
 	/* Ensure not reading past end of the image */
-	if (*ppos + count > image_size)
-		count = image_size - *ppos;
+	if (count > image_size - offset)
+		count = image_size - offset;
 
 	switch (type[minor]) {
 	case MASTER_MINOR:
@@ -223,6 +236,7 @@ static ssize_t vme_user_write(struct file *file, const char __user *buf,
 {
 	unsigned int minor = iminor(file_inode(file));
 	ssize_t retval;
+	size_t offset;
 	size_t image_size;
 
 	if (minor == CONTROL_MINOR)
@@ -230,17 +244,19 @@ static ssize_t vme_user_write(struct file *file, const char __user *buf,
 
 	mutex_lock(&image[minor].mutex);
 
-	image_size = vme_get_size(image[minor].resource);
+	image_size = vme_user_get_image_size(minor);
 
 	/* Ensure we are starting at a valid location */
-	if ((*ppos < 0) || (*ppos > (image_size - 1))) {
+	if ((*ppos < 0) || ((u64)*ppos >= image_size)) {
 		mutex_unlock(&image[minor].mutex);
 		return 0;
 	}
 
+	offset = *ppos;
+
 	/* Ensure not reading past end of the image */
-	if (*ppos + count > image_size)
-		count = image_size - *ppos;
+	if (count > image_size - offset)
+		count = image_size - offset;
 
 	switch (type[minor]) {
 	case MASTER_MINOR:
@@ -271,7 +287,7 @@ static loff_t vme_user_llseek(struct file *file, loff_t off, int whence)
 	case MASTER_MINOR:
 	case SLAVE_MINOR:
 		mutex_lock(&image[minor].mutex);
-		image_size = vme_get_size(image[minor].resource);
+		image_size = vme_user_get_image_size(minor);
 		res = fixed_size_llseek(file, off, whence, image_size);
 		mutex_unlock(&image[minor].mutex);
 		return res;
@@ -394,6 +410,10 @@ static int vme_user_ioctl(struct inode *inode, struct file *file,
 				return -EFAULT;
 			}
 
+			if (slave.enable &&
+			    (!slave.size || slave.size > image[minor].size_buf))
+				return -EINVAL;
+
 			/* XXX	We do not want to push aspace, cycle and width
 			 *	to userspace as they are
 			 */
-- 
2.54.0

Re: [PATCH] staging: vme_user: bound slave windows to DMA buffers

[Greg Kroah-Hartman]

On Thu, Jun 25, 2026 at 11:31:37AM +0200, Yousef Alhouseen wrote:
> vme_user allocates a fixed PCI_BUF_SIZE coherent DMA buffer for each
> slave image, but VME_SET_SLAVE passes the user-supplied size to
> vme_slave_set() unchanged. An enabled window larger than PCI_BUF_SIZE lets
> the bridge expose DMA addresses beyond the allocation.
> 
> The character device read/write paths also derive their bounds from
> vme_get_size(), so the oversized programmed window can make
> buffer_to_user() and buffer_from_user() access past
> image[minor].kern_buf.
> 
> Reject enabled slave windows that do not fit in the backing buffer and use
> the same capped size for slave read/write/llseek bounds. Also convert the
> read/write limit checks to subtraction-based form so offset + count cannot
> wrap around the image size check.
> 
> Signed-off-by: Yousef Alhouseen <alhouseenyousef@gmail.com>
> ---
>  drivers/staging/vme_user/vme_user.c | 38 ++++++++++++++++++++++-------
>  1 file changed, 29 insertions(+), 9 deletions(-)

What tool did you use to find/make this change, and how was it tested?

thanks,

greg k-h

Re: [PATCH] staging: vme_user: bound slave windows to DMA buffers

[Dan Carpenter]

There are two bug fixes in this patch:

1) The driver allows you to set any value for size.
2) The vme_get_size() function returns zero on error but nothing
   checks for error.

There is a third bug as well which is not addressed.

3) The slave.vme_addr is not checked and it allows you to read/write
   to any memory.

It sort of looks like bugs 1 and 2 only affect
drivers/staging/vme_user/vme_fake.c.  I feel like people hacked
together some debug code and weren't too worried about the security
since it wasn't intended to be used on real systems...  The temptation
is to fix this by deleting at least vme_fake.c.

On Thu, Jun 25, 2026 at 11:31:37AM +0200, Yousef Alhouseen wrote:
> vme_user allocates a fixed PCI_BUF_SIZE coherent DMA buffer for each
> slave image, but VME_SET_SLAVE passes the user-supplied size to
> vme_slave_set() unchanged.

Obviously you meant checked instead of changed...  There is some
checking in vme_check_window() but it's not suffient.

> An enabled window larger than PCI_BUF_SIZE lets
> the bridge expose DMA addresses beyond the allocation.
> 
> The character device read/write paths also derive their bounds from
> vme_get_size(), so the oversized programmed window can make
> buffer_to_user() and buffer_from_user() access past
> image[minor].kern_buf.
> 
> Reject enabled slave windows that do not fit in the backing buffer and use
> the same capped size for slave read/write/llseek bounds. Also convert the
> read/write limit checks to subtraction-based form so offset + count cannot
> wrap around the image size check.

An "also" in the commit message, means split it out into its own patch.

> 
> Signed-off-by: Yousef Alhouseen <alhouseenyousef@gmail.com>
> ---

Fixes tag.  Please say when you are using AI.

>  drivers/staging/vme_user/vme_user.c | 38 ++++++++++++++++++++++-------
>  1 file changed, 29 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/staging/vme_user/vme_user.c b/drivers/staging/vme_user/vme_user.c
> index 11e25c2f6..24d3c6ec3 100644
> --- a/drivers/staging/vme_user/vme_user.c
> +++ b/drivers/staging/vme_user/vme_user.c
> @@ -175,11 +175,22 @@ static ssize_t buffer_from_user(unsigned int minor, const char __user *buf,
>  	return count;
>  }
>  
> +static size_t vme_user_get_image_size(unsigned int minor)
> +{
> +	size_t image_size = vme_get_size(image[minor].resource);
> +
> +	if (type[minor] == SLAVE_MINOR)
> +		image_size = min_t(size_t, image_size, image[minor].size_buf);
> +
> +	return image_size;
> +}

No, vme_get_size() should return the correct size.  If it's really a
bug then fix that instead of adding a work-around here.

> +
>  static ssize_t vme_user_read(struct file *file, char __user *buf, size_t count,
>  			     loff_t *ppos)
>  {
>  	unsigned int minor = iminor(file_inode(file));
>  	ssize_t retval;
> +	size_t offset;

No need for this variable.

>  	size_t image_size;
>  
>  	if (minor == CONTROL_MINOR)
> @@ -188,17 +199,19 @@ static ssize_t vme_user_read(struct file *file, char __user *buf, size_t count,
>  	mutex_lock(&image[minor].mutex);
>  
>  	/* XXX Do we *really* want this helper - we can use vme_*_get ? */
> -	image_size = vme_get_size(image[minor].resource);
> +	image_size = vme_user_get_image_size(minor);

vme_get_size() returns zero on error.  Just add an check for errors:

	if (image_size == 0) {
		mutex_unlock(&image[minor].mutex);
		return 0;
	}

>  
>  	/* Ensure we are starting at a valid location */
> -	if ((*ppos < 0) || (*ppos > (image_size - 1))) {
> +	if ((*ppos < 0) || ((u64)*ppos >= image_size)) {

No need for this cast.

	if ((*ppos < 0) || (*ppos >= image_size)) {

But actually if we check for zero then this is unnecesssary.

>  		mutex_unlock(&image[minor].mutex);
>  		return 0;
>  	}
>  
> +	offset = *ppos;
> +
>  	/* Ensure not reading past end of the image */
> -	if (*ppos + count > image_size)
> -		count = image_size - *ppos;
> +	if (count > image_size - offset)
> +		count = image_size - offset;

The kernel ensures that "*ppos + count" can't overflow in
rw_verify_area() so this change is unnecessary.

>  
>  	switch (type[minor]) {
>  	case MASTER_MINOR:
> @@ -223,6 +236,7 @@ static ssize_t vme_user_write(struct file *file, const char __user *buf,
>  {
>  	unsigned int minor = iminor(file_inode(file));
>  	ssize_t retval;
> +	size_t offset;
>  	size_t image_size;
>  
>  	if (minor == CONTROL_MINOR)
> @@ -230,17 +244,19 @@ static ssize_t vme_user_write(struct file *file, const char __user *buf,
>  
>  	mutex_lock(&image[minor].mutex);
>  
> -	image_size = vme_get_size(image[minor].resource);
> +	image_size = vme_user_get_image_size(minor);
>  
>  	/* Ensure we are starting at a valid location */
> -	if ((*ppos < 0) || (*ppos > (image_size - 1))) {
> +	if ((*ppos < 0) || ((u64)*ppos >= image_size)) {
>  		mutex_unlock(&image[minor].mutex);
>  		return 0;
>  	}
>  
> +	offset = *ppos;
> +
>  	/* Ensure not reading past end of the image */
> -	if (*ppos + count > image_size)
> -		count = image_size - *ppos;
> +	if (count > image_size - offset)
> +		count = image_size - offset;
>  
>  	switch (type[minor]) {
>  	case MASTER_MINOR:
> @@ -271,7 +287,7 @@ static loff_t vme_user_llseek(struct file *file, loff_t off, int whence)
>  	case MASTER_MINOR:
>  	case SLAVE_MINOR:
>  		mutex_lock(&image[minor].mutex);
> -		image_size = vme_get_size(image[minor].resource);
> +		image_size = vme_user_get_image_size(minor);
>  		res = fixed_size_llseek(file, off, whence, image_size);
>  		mutex_unlock(&image[minor].mutex);
>  		return res;
> @@ -394,6 +410,10 @@ static int vme_user_ioctl(struct inode *inode, struct file *file,
>  				return -EFAULT;
>  			}
>  
> +			if (slave.enable &&
> +			    (!slave.size || slave.size > image[minor].size_buf))
> +				return -EINVAL;

This isn't the right place to check...  It should probably be done in
vme_check_window()?

This code is quite tricky and I don't really understand it all.  Why does
resource_to/from_user() check for buffer overflow but buffer_to/from_user()
does not?  The checking in resource_to/from_user() is also insufficient
because it doesn't take *ppos into consideration but fortunately,
vme_master_read/write() does.  This code would be safer if we added
bounds checking to buffer_to/from_user().

regards,
dan carpenter





> +
>  			/* XXX	We do not want to push aspace, cycle and width
>  			 *	to userspace as they are
>  			 */
> -- 
> 2.54.0
> 

Re: [PATCH] staging: vme_user: bound slave windows to DMA buffers

[Dan Carpenter]

On Thu, Jun 25, 2026 at 02:51:55PM +0300, Dan Carpenter wrote:
> There are two bug fixes in this patch:
> 
> 1) The driver allows you to set any value for size.
> 2) The vme_get_size() function returns zero on error but nothing
>    checks for error.
> 
> There is a third bug as well which is not addressed.
> 
> 3) The slave.vme_addr is not checked and it allows you to read/write
>    to any memory.
> 
> It sort of looks like bugs 1 and 2 only affect
> drivers/staging/vme_user/vme_fake.c.

Actually, no.  The bugs seem to affect drivers/staging/vme_user/vme_tsi148.c
but the variables are stored to registers instead of variables so it
was a little more convoluted.

regards,
dan carpenter

Re: [PATCH] staging: vme_user: bound slave windows to DMA buffers

[Yousef Alhouseen]

Hi Dan, Greg,

Thanks for going through it. Agreed, this version mixes separate fixes
and the helper is the wrong shape. Please drop it.

I found it while reading the vme_user ioctl and read/write paths.
Testing was only git diff --check and checkpatch; I don't have VME
hardware.

I'll rework from the core/bridge side and split out anything that
still holds up.

Thanks,
Yousef

On Thu, 25 Jun 2026 14:58:14 +0300, Dan Carpenter <error27@gmail.com> wrote:
> On Thu, Jun 25, 2026 at 02:51:55PM +0300, Dan Carpenter wrote:
> > There are two bug fixes in this patch:
> >
> > 1) The driver allows you to set any value for size.
> > 2) The vme_get_size() function returns zero on error but nothing
> > checks for error.
> >
> > There is a third bug as well which is not addressed.
> >
> > 3) The slave.vme_addr is not checked and it allows you to read/write
> > to any memory.
> >
> > It sort of looks like bugs 1 and 2 only affect
> > drivers/staging/vme_user/vme_fake.c.
>
> Actually, no. The bugs seem to affect drivers/staging/vme_user/vme_tsi148.c
> but the variables are stored to registers instead of variables so it
> was a little more convoluted.
>
> regards,
> dan carpenter