* [PATCH] vme_user: tighten slave window bounds and validate offsets
@ 2026-05-27 21:14 Lucas Faria Mendes
2026-05-28 5:33 ` Greg KH
2026-05-28 7:00 ` Dan Carpenter
0 siblings, 2 replies; 3+ messages in thread
From: Lucas Faria Mendes @ 2026-05-27 21:14 UTC (permalink / raw)
To: gregkh; +Cc: linux-kernel, linux-staging, 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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] vme_user: tighten slave window bounds and validate offsets
2026-05-27 21:14 [PATCH] vme_user: tighten slave window bounds and validate offsets Lucas Faria Mendes
@ 2026-05-28 5:33 ` Greg KH
2026-05-28 7:00 ` Dan Carpenter
1 sibling, 0 replies; 3+ messages in thread
From: Greg KH @ 2026-05-28 5:33 UTC (permalink / raw)
To: Lucas Faria Mendes; +Cc: linux-kernel, linux-staging
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] vme_user: tighten slave window bounds and validate offsets
2026-05-27 21:14 [PATCH] vme_user: tighten slave window bounds and validate offsets Lucas Faria Mendes
2026-05-28 5:33 ` Greg KH
@ 2026-05-28 7:00 ` Dan Carpenter
1 sibling, 0 replies; 3+ messages in thread
From: Dan Carpenter @ 2026-05-28 7:00 UTC (permalink / raw)
To: Lucas Faria Mendes; +Cc: gregkh, linux-kernel, linux-staging
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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2026-05-28 7:00 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-05-27 21:14 [PATCH] vme_user: tighten slave window bounds and validate offsets Lucas Faria Mendes
2026-05-28 5:33 ` Greg KH
2026-05-28 7:00 ` Dan Carpenter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox