diff mbox series

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

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

Commit Message

Ivan Stepchenko May 5, 2025, 8:13 a.m. UTC
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(-)

Comments

Lee Jones May 8, 2025, 2:34 p.m. UTC | #1
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
>
David Lechner May 8, 2025, 3:03 p.m. UTC | #2
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
>>
>
diff mbox series

Patch

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