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@xxxxxxxxx>

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