[PATCH] vme_user: tighten slave window bounds and validate offsets

[PATCH] vme_user: tighten slave window bounds and validate offsets

[Lucas Faria Mendes]

Limit slave read/write operations to the allocated buffer size to
prevent out-of-bounds access when a slave window is
configured larger than its buffer.
Treat zero-sized windows and invalid file positions as errors,
and reject VME_SET_SLAVE requests that exceed the slave buffer.
This makes the user access driver more robust and avoids
silent EOF on invalid offsets.

Signed-off-by: Lucas Faria Mendes <lucas.fariamo08@gmail.com>

diff --git a/drivers/staging/vme_user/vme_user.c b/drivers/staging/vme_user/vme_user.c
index 11e25c2f6..ca65cb57c 100644
--- a/drivers/staging/vme_user/vme_user.c
+++ b/drivers/staging/vme_user/vme_user.c
@@ -181,6 +181,7 @@ static ssize_t vme_user_read(struct file *file, char __user *buf, size_t count,
 	unsigned int minor = iminor(file_inode(file));
 	ssize_t retval;
 	size_t image_size;
+	size_t max_size;

 	if (minor == CONTROL_MINOR)
 		return 0;
@@ -189,16 +190,24 @@ static ssize_t vme_user_read(struct file *file, char __user *buf, size_t count,

 	/* XXX Do we *really* want this helper - we can use vme_*_get ? */
 	image_size = vme_get_size(image[minor].resource);
+	if (!image_size) {
+		mutex_unlock(&image[minor].mutex);
+		return -EINVAL;
+	}
+
+	max_size = image_size;
+	if (type[minor] == SLAVE_MINOR && max_size > image[minor].size_buf)
+		max_size = image[minor].size_buf;

 	/* Ensure we are starting at a valid location */
-	if ((*ppos < 0) || (*ppos > (image_size - 1))) {
+	if ((*ppos < 0) || (*ppos >= max_size)) {
 		mutex_unlock(&image[minor].mutex);
-		return 0;
+		return -EINVAL;
 	}

 	/* Ensure not reading past end of the image */
-	if (*ppos + count > image_size)
-		count = image_size - *ppos;
+	if (*ppos + count > max_size)
+		count = max_size - *ppos;

 	switch (type[minor]) {
 	case MASTER_MINOR:
@@ -224,6 +233,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 image_size;
+	size_t max_size;

 	if (minor == CONTROL_MINOR)
 		return 0;
@@ -231,16 +241,24 @@ 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);
+	if (!image_size) {
+		mutex_unlock(&image[minor].mutex);
+		return -EINVAL;
+	}
+
+	max_size = image_size;
+	if (type[minor] == SLAVE_MINOR && max_size > image[minor].size_buf)
+		max_size = image[minor].size_buf;

 	/* Ensure we are starting at a valid location */
-	if ((*ppos < 0) || (*ppos > (image_size - 1))) {
+	if ((*ppos < 0) || (*ppos >= max_size)) {
 		mutex_unlock(&image[minor].mutex);
-		return 0;
+		return -EINVAL;
 	}

 	/* Ensure not reading past end of the image */
-	if (*ppos + count > image_size)
-		count = image_size - *ppos;
+	if (*ppos + count > max_size)
+		count = max_size - *ppos;

 	switch (type[minor]) {
 	case MASTER_MINOR:
@@ -394,6 +412,9 @@ static int vme_user_ioctl(struct inode *inode, struct file *file,
 				return -EFAULT;
 			}

+			if (slave.size > image[minor].size_buf)
+				return -EINVAL;
+
 			/* XXX	We do not want to push aspace, cycle and width
 			 *	to userspace as they are
 			 */
---
 drivers/staging/vme_user/vme_user.c | 37 ++++++++++++++++++++++-------
 1 file changed, 29 insertions(+), 8 deletions(-)

diff --git a/drivers/staging/vme_user/vme_user.c b/drivers/staging/vme_user/vme_user.c
index 11e25c2f6..ca65cb57c 100644
--- a/drivers/staging/vme_user/vme_user.c
+++ b/drivers/staging/vme_user/vme_user.c
@@ -181,6 +181,7 @@ static ssize_t vme_user_read(struct file *file, char __user *buf, size_t count,
 	unsigned int minor = iminor(file_inode(file));
 	ssize_t retval;
 	size_t image_size;
+	size_t max_size;
 
 	if (minor == CONTROL_MINOR)
 		return 0;
@@ -189,16 +190,24 @@ static ssize_t vme_user_read(struct file *file, char __user *buf, size_t count,
 
 	/* XXX Do we *really* want this helper - we can use vme_*_get ? */
 	image_size = vme_get_size(image[minor].resource);
+	if (!image_size) {
+		mutex_unlock(&image[minor].mutex);
+		return -EINVAL;
+	}
+
+	max_size = image_size;
+	if (type[minor] == SLAVE_MINOR && max_size > image[minor].size_buf)
+		max_size = image[minor].size_buf;
 
 	/* Ensure we are starting at a valid location */
-	if ((*ppos < 0) || (*ppos > (image_size - 1))) {
+	if ((*ppos < 0) || (*ppos >= max_size)) {
 		mutex_unlock(&image[minor].mutex);
-		return 0;
+		return -EINVAL;
 	}
 
 	/* Ensure not reading past end of the image */
-	if (*ppos + count > image_size)
-		count = image_size - *ppos;
+	if (*ppos + count > max_size)
+		count = max_size - *ppos;
 
 	switch (type[minor]) {
 	case MASTER_MINOR:
@@ -224,6 +233,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 image_size;
+	size_t max_size;
 
 	if (minor == CONTROL_MINOR)
 		return 0;
@@ -231,16 +241,24 @@ 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);
+	if (!image_size) {
+		mutex_unlock(&image[minor].mutex);
+		return -EINVAL;
+	}
+
+	max_size = image_size;
+	if (type[minor] == SLAVE_MINOR && max_size > image[minor].size_buf)
+		max_size = image[minor].size_buf;
 
 	/* Ensure we are starting at a valid location */
-	if ((*ppos < 0) || (*ppos > (image_size - 1))) {
+	if ((*ppos < 0) || (*ppos >= max_size)) {
 		mutex_unlock(&image[minor].mutex);
-		return 0;
+		return -EINVAL;
 	}
 
 	/* Ensure not reading past end of the image */
-	if (*ppos + count > image_size)
-		count = image_size - *ppos;
+	if (*ppos + count > max_size)
+		count = max_size - *ppos;
 
 	switch (type[minor]) {
 	case MASTER_MINOR:
@@ -394,6 +412,9 @@ static int vme_user_ioctl(struct inode *inode, struct file *file,
 				return -EFAULT;
 			}
 
+			if (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.53.0

Re: [PATCH] vme_user: tighten slave window bounds and validate offsets

[Greg KH]

On Wed, May 27, 2026 at 06:14:24PM -0300, Lucas Faria Mendes wrote:
> Limit slave read/write operations to the allocated buffer size to
> prevent out-of-bounds access when a slave window is
> configured larger than its buffer.
> Treat zero-sized windows and invalid file positions as errors,
> and reject VME_SET_SLAVE requests that exceed the slave buffer.
> This makes the user access driver more robust and avoids
> silent EOF on invalid offsets.

Odd line-wrapping :(

How did you find this issue?  How was it tested?

thanks,

greg k-h

Re: [PATCH] vme_user: tighten slave window bounds and validate offsets

[Dan Carpenter]

On Wed, May 27, 2026 at 06:14:24PM -0300, Lucas Faria Mendes wrote:
> Limit slave read/write operations to the allocated buffer size to
> prevent out-of-bounds access when a slave window is
> configured larger than its buffer.
> Treat zero-sized windows and invalid file positions as errors,
> and reject VME_SET_SLAVE requests that exceed the slave buffer.
> This makes the user access driver more robust and avoids
> silent EOF on invalid offsets.
> 
> Signed-off-by: Lucas Faria Mendes <lucas.fariamo08@gmail.com>

No Fixes tag.

This is a grab bag of changes.  It needs to be split up into
multiple changes.

> 
> diff --git a/drivers/staging/vme_user/vme_user.c b/drivers/staging/vme_user/vme_user.c
> index 11e25c2f6..ca65cb57c 100644
> --- a/drivers/staging/vme_user/vme_user.c
> +++ b/drivers/staging/vme_user/vme_user.c
> @@ -181,6 +181,7 @@ static ssize_t vme_user_read(struct file *file, char __user *buf, size_t count,
>  	unsigned int minor = iminor(file_inode(file));
>  	ssize_t retval;
>  	size_t image_size;
> +	size_t max_size;
> 
>  	if (minor == CONTROL_MINOR)
>  		return 0;
> @@ -189,16 +190,24 @@ static ssize_t vme_user_read(struct file *file, char __user *buf, size_t count,
> 
>  	/* XXX Do we *really* want this helper - we can use vme_*_get ? */
>  	image_size = vme_get_size(image[minor].resource);
> +	if (!image_size) {
> +		mutex_unlock(&image[minor].mutex);
> +		return -EINVAL;

The original code looks pretty shady.  Sure.  This is fine.

> +	}
> +
> +	max_size = image_size;
> +	if (type[minor] == SLAVE_MINOR && max_size > image[minor].size_buf)
> +		max_size = image[minor].size_buf;

I don't understand this chunk.  It seems unrelated to a zero
return from vme_get_size().  It needs to be in a separate patch.

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

Fine.  The zero size - 1 is no good.

>  		mutex_unlock(&image[minor].mutex);
> -		return 0;
> +		return -EINVAL;

No.  You can't change this because it's API.  Also unrelated.

>  	}
> 
>  	/* Ensure not reading past end of the image */
> -	if (*ppos + count > image_size)
> -		count = image_size - *ppos;
> +	if (*ppos + count > max_size)
> +		count = max_size - *ppos;

This is unrelated to zero size so it would go in the other patch.


> 
>  	switch (type[minor]) {
>  	case MASTER_MINOR:
> @@ -224,6 +233,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 image_size;
> +	size_t max_size;
> 
>  	if (minor == CONTROL_MINOR)
>  		return 0;
> @@ -231,16 +241,24 @@ 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);
> +	if (!image_size) {
> +		mutex_unlock(&image[minor].mutex);
> +		return -EINVAL;
> +	}
> +
> +	max_size = image_size;
> +	if (type[minor] == SLAVE_MINOR && max_size > image[minor].size_buf)
> +		max_size = image[minor].size_buf;
> 
>  	/* Ensure we are starting at a valid location */
> -	if ((*ppos < 0) || (*ppos > (image_size - 1))) {
> +	if ((*ppos < 0) || (*ppos >= max_size)) {
>  		mutex_unlock(&image[minor].mutex);
> -		return 0;
> +		return -EINVAL;
>  	}
> 
>  	/* Ensure not reading past end of the image */
> -	if (*ppos + count > image_size)
> -		count = image_size - *ppos;
> +	if (*ppos + count > max_size)
> +		count = max_size - *ppos;
> 
>  	switch (type[minor]) {
>  	case MASTER_MINOR:

Same review comments.

> @@ -394,6 +412,9 @@ static int vme_user_ioctl(struct inode *inode, struct file *file,
>  				return -EFAULT;
>  			}
> 
> +			if (slave.size > image[minor].size_buf)
> +				return -EINVAL;

This change is not explained well.  It would need to be in it's own
commit message.

The size is supposed to be checked in vme_check_window() so this
is the wrong place.  I looked at the checking and it seems okay
to me.

The rest is just more of the same.

regards,
dan carpenter