| Message ID | 20250505081342.3855-1-sid@itb.spb.ru |
|---|---|
| State | New |
| Headers | show |
| Series | leds: uleds: fix unchecked copy_to_user() in uleds_read() | expand |
On Mon, 05 May 2025, Ivan Stepchenko wrote: > The copy_to_user() is annotated with __must_check, indicating that > its return value must be checked by the caller. Currently, uleds_read() > ignores it. If the userspace buffer is invalid and copy_to_user() fails, > the userspace application may assume it has received fresh data, while > in fact copying has failed. This can leave applications out of sync > with the actual device state. > > Found by Linux Verification Center (linuxtesting.org) with SVACE. > > Fixes: e381322b0190 ("leds: Introduce userspace LED class driver") > Signed-off-by: Ivan Stepchenko <sid@itb.spb.ru> > --- > drivers/leds/uleds.c | 11 +++++++---- > 1 file changed, 7 insertions(+), 4 deletions(-) > > diff --git a/drivers/leds/uleds.c b/drivers/leds/uleds.c > index 374a841f18c3..41bfce43136c 100644 > --- a/drivers/leds/uleds.c > +++ b/drivers/leds/uleds.c > @@ -147,10 +147,13 @@ static ssize_t uleds_read(struct file *file, char __user *buffer, size_t count, > } else if (!udev->new_data && (file->f_flags & O_NONBLOCK)) { > retval = -EAGAIN; > } else if (udev->new_data) { > - retval = copy_to_user(buffer, &udev->brightness, > - sizeof(udev->brightness)); > - udev->new_data = false; > - retval = sizeof(udev->brightness); > + if (copy_to_user(buffer, &udev->brightness, > + sizeof(udev->brightness))) { This is not good. Please store the result into a variable and return that instead. > + retval = -EFAULT; > + } else { > + udev->new_data = false; > + retval = sizeof(udev->brightness); > + } > } > > mutex_unlock(&udev->mutex); > -- > 2.39.5 >
On 5/8/25 9:34 AM, Lee Jones wrote: > On Mon, 05 May 2025, Ivan Stepchenko wrote: > >> The copy_to_user() is annotated with __must_check, indicating that >> its return value must be checked by the caller. Currently, uleds_read() >> ignores it. If the userspace buffer is invalid and copy_to_user() fails, >> the userspace application may assume it has received fresh data, while >> in fact copying has failed. This can leave applications out of sync >> with the actual device state. >> >> Found by Linux Verification Center (linuxtesting.org) with SVACE. >> >> Fixes: e381322b0190 ("leds: Introduce userspace LED class driver") >> Signed-off-by: Ivan Stepchenko <sid@itb.spb.ru> >> --- >> drivers/leds/uleds.c | 11 +++++++---- >> 1 file changed, 7 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/leds/uleds.c b/drivers/leds/uleds.c >> index 374a841f18c3..41bfce43136c 100644 >> --- a/drivers/leds/uleds.c >> +++ b/drivers/leds/uleds.c >> @@ -147,10 +147,13 @@ static ssize_t uleds_read(struct file *file, char __user *buffer, size_t count, >> } else if (!udev->new_data && (file->f_flags & O_NONBLOCK)) { >> retval = -EAGAIN; >> } else if (udev->new_data) { >> - retval = copy_to_user(buffer, &udev->brightness, >> - sizeof(udev->brightness)); >> - udev->new_data = false; >> - retval = sizeof(udev->brightness); >> + if (copy_to_user(buffer, &udev->brightness, >> + sizeof(udev->brightness))) { > > This is not good. > > Please store the result into a variable and return that instead. Every other caller of copy_to_user() in the kernel I've seen ignores the actual return value and returns -EFAULT, so I thought this looked correct and it was just a quirk of copy_to_user(). > >> + retval = -EFAULT; >> + } else { >> + udev->new_data = false; >> + retval = sizeof(udev->brightness); >> + } >> } >> >> mutex_unlock(&udev->mutex); >> -- >> 2.39.5 >> >
On Thu, 08 May 2025, David Lechner wrote: > On 5/8/25 9:34 AM, Lee Jones wrote: > > On Mon, 05 May 2025, Ivan Stepchenko wrote: > > > >> The copy_to_user() is annotated with __must_check, indicating that > >> its return value must be checked by the caller. Currently, uleds_read() > >> ignores it. If the userspace buffer is invalid and copy_to_user() fails, > >> the userspace application may assume it has received fresh data, while > >> in fact copying has failed. This can leave applications out of sync > >> with the actual device state. > >> > >> Found by Linux Verification Center (linuxtesting.org) with SVACE. > >> > >> Fixes: e381322b0190 ("leds: Introduce userspace LED class driver") > >> Signed-off-by: Ivan Stepchenko <sid@itb.spb.ru> > >> --- > >> drivers/leds/uleds.c | 11 +++++++---- > >> 1 file changed, 7 insertions(+), 4 deletions(-) > >> > >> diff --git a/drivers/leds/uleds.c b/drivers/leds/uleds.c > >> index 374a841f18c3..41bfce43136c 100644 > >> --- a/drivers/leds/uleds.c > >> +++ b/drivers/leds/uleds.c > >> @@ -147,10 +147,13 @@ static ssize_t uleds_read(struct file *file, char __user *buffer, size_t count, > >> } else if (!udev->new_data && (file->f_flags & O_NONBLOCK)) { > >> retval = -EAGAIN; > >> } else if (udev->new_data) { > >> - retval = copy_to_user(buffer, &udev->brightness, > >> - sizeof(udev->brightness)); > >> - udev->new_data = false; > >> - retval = sizeof(udev->brightness); > >> + if (copy_to_user(buffer, &udev->brightness, > >> + sizeof(udev->brightness))) { > > > > This is not good. > > > > Please store the result into a variable and return that instead. > > Every other caller of copy_to_user() in the kernel I've seen ignores the actual > return value and returns -EFAULT, so I thought this looked correct and it was > just a quirk of copy_to_user(). Yes, I think you're right. Interesting. So my counterproposal is as follows: --- a/drivers/leds/uleds.c +++ b/drivers/leds/uleds.c @@ -147,10 +147,11 @@ static ssize_t uleds_read(struct file *file, char __user *buffer, size_t count, } else if (!udev->new_data && (file->f_flags & O_NONBLOCK)) { retval = -EAGAIN; } else if (udev->new_data) { - retval = copy_to_user(buffer, &udev->brightness, - sizeof(udev->brightness)); - udev->new_data = false; - retval = sizeof(udev->brightness); + ssize_t size = retval = sizeof(udev->brightness); + if (copy_to_user(buffer, &udev->brightness, size)) + retval = -EFAULT; + else + udev->new_data = false; } mutex_unlock(&udev->mutex); > >> + retval = -EFAULT; > >> + } else { > >> + udev->new_data = false; > >> + retval = sizeof(udev->brightness); > >> + } > >> } > >> > >> mutex_unlock(&udev->mutex); > >> -- > >> 2.39.5 > >> > > >
diff --git a/drivers/leds/uleds.c b/drivers/leds/uleds.c index 374a841f18c3..41bfce43136c 100644 --- a/drivers/leds/uleds.c +++ b/drivers/leds/uleds.c @@ -147,10 +147,13 @@ static ssize_t uleds_read(struct file *file, char __user *buffer, size_t count, } else if (!udev->new_data && (file->f_flags & O_NONBLOCK)) { retval = -EAGAIN; } else if (udev->new_data) { - retval = copy_to_user(buffer, &udev->brightness, - sizeof(udev->brightness)); - udev->new_data = false; - retval = sizeof(udev->brightness); + if (copy_to_user(buffer, &udev->brightness, + sizeof(udev->brightness))) { + retval = -EFAULT; + } else { + udev->new_data = false; + retval = sizeof(udev->brightness); + } } mutex_unlock(&udev->mutex);
The copy_to_user() is annotated with __must_check, indicating that its return value must be checked by the caller. Currently, uleds_read() ignores it. If the userspace buffer is invalid and copy_to_user() fails, the userspace application may assume it has received fresh data, while in fact copying has failed. This can leave applications out of sync with the actual device state. Found by Linux Verification Center (linuxtesting.org) with SVACE. Fixes: e381322b0190 ("leds: Introduce userspace LED class driver") Signed-off-by: Ivan Stepchenko <sid@itb.spb.ru> --- drivers/leds/uleds.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)