leds: uleds: fix unchecked copy_to_user() in uleds_read()

[PATCH] leds: uleds: fix unchecked copy_to_user() in uleds_read()

Posted by Ivan Stepchenko 9 months, 1 week ago

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))) {
+				retval = -EFAULT;
+			} else {
+				udev->new_data = false;
+				retval = sizeof(udev->brightness);
+			}
 		}
 
 		mutex_unlock(&udev->mutex);
-- 
2.39.5

Re: [PATCH] leds: uleds: fix unchecked copy_to_user() in uleds_read()

Posted by Lee Jones 9 months ago

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
> 

-- 
Lee Jones [李琼斯]

Re: [PATCH] leds: uleds: fix unchecked copy_to_user() in uleds_read()

Posted by David Lechner 9 months ago

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

Re: [PATCH] leds: uleds: fix unchecked copy_to_user() in uleds_read()

Posted by Lee Jones 9 months ago

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

-- 
Lee Jones [李琼斯]