diff mbox series

[v2] Updating a vulnerable use of strcpy.

Message ID 20240702185523.17716-1-qasim.majeed20@gmail.com
State Superseded
Headers show
Series [v2] Updating a vulnerable use of strcpy. | expand

Commit Message

Muhammad Qasim Abdul Majeed July 2, 2024, 6:55 p.m. UTC
Replacing strcpy with strscpy and memory bound the copy. strcpy is a deprecated function. It should be removed from the kernel source.

Reference: https://github.com/KSPP/linux/issues/88

Signed-off-by: Muhammad Qasim Abdul Majeed <qasim.majeed20@gmail.com>

> In what way exactly is it vulnerable?
strcpy is a deprecated interface (reference: https://github.com/KSPP/linux/issues/88). It should be removed from kernel source.
It is reported as vulnerable in Enabling Linux in Safety Critical Applications (ELISA) builder.

> Why is a runtime check needed here if all of the sizes in question are known at compile time?
Runtime check has been replaced with compile time check.

---
v1 -> v2: Commit message has been updated and runtime check is replace with compile time check.

 drivers/acpi/acpi_video.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Comments

Rafael J. Wysocki July 2, 2024, 7:28 p.m. UTC | #1
On Tue, Jul 2, 2024 at 9:04 PM Muhammad Qasim Abdul Majeed
<qasim.majeed20@gmail.com> wrote:
>
> Replacing strcpy with strscpy and memory bound the copy.

Why?  In this particular case, it is not fundamentally necessary.

> strcpy is a deprecated function. It should be removed from the kernel source.

If the goal is to get rid of all strcpy() calls from the kernel
because using it is generally unsafe, just say so in the changelog and
it will be fine.

> Reference: https://github.com/KSPP/linux/issues/88
>
> Signed-off-by: Muhammad Qasim Abdul Majeed <qasim.majeed20@gmail.com>
>
> > In what way exactly is it vulnerable?
> strcpy is a deprecated interface (reference: https://github.com/KSPP/linux/issues/88). It should be removed from kernel source.
> It is reported as vulnerable in Enabling Linux in Safety Critical Applications (ELISA) builder.
>
> > Why is a runtime check needed here if all of the sizes in question are known at compile time?
> Runtime check has been replaced with compile time check.
>
> ---
> v1 -> v2: Commit message has been updated and runtime check is replace with compile time check.
>
>  drivers/acpi/acpi_video.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c
> index 1fda30388297..be8346a66374 100644
> --- a/drivers/acpi/acpi_video.c
> +++ b/drivers/acpi/acpi_video.c
> @@ -1128,8 +1128,8 @@ static int acpi_video_bus_get_one_device(struct acpi_device *device, void *arg)
>                 return -ENOMEM;
>         }
>
> -       strcpy(acpi_device_name(device), ACPI_VIDEO_DEVICE_NAME);
> -       strcpy(acpi_device_class(device), ACPI_VIDEO_CLASS);
> +       strscpy(acpi_device_name(device), ACPI_VIDEO_DEVICE_NAME, sizeof(ACPI_VIDEO_DEVICE_NAME));
> +       strscpy(acpi_device_class(device), ACPI_VIDEO_CLASS, sizeof(ACPI_VIDEO_CLASS));

The strscpy() kerneldoc comment says this:

 * The size argument @... is only required when @dst is not an array, or
 * when the copy needs to be smaller than sizeof(@dst).

So is it necessary to use the size argument here and below?

>         data->device_id = device_id;
>         data->video = video;
> @@ -2010,8 +2010,8 @@ static int acpi_video_bus_add(struct acpi_device *device)
>         }
>
>         video->device = device;
> -       strcpy(acpi_device_name(device), ACPI_VIDEO_BUS_NAME);
> -       strcpy(acpi_device_class(device), ACPI_VIDEO_CLASS);
> +       strscpy(acpi_device_name(device), ACPI_VIDEO_BUS_NAME, sizeof(ACPI_VIDEO_BUS_NAME));
> +       strscpy(acpi_device_class(device), ACPI_VIDEO_CLASS, sizeof(ACPI_VIDEO_CLASS));
>         device->driver_data = video;
>
>         acpi_video_bus_find_cap(video);
> --
Christophe JAILLET July 2, 2024, 7:29 p.m. UTC | #2
Le 02/07/2024 à 20:55, Muhammad Qasim Abdul Majeed a écrit :
> Replacing strcpy with strscpy and memory bound the copy. strcpy is a deprecated function. It should be removed from the kernel source.
> 
> Reference: https://github.com/KSPP/linux/issues/88
> 
> Signed-off-by: Muhammad Qasim Abdul Majeed <qasim.majeed20@gmail.com>
> 
>> In what way exactly is it vulnerable?
> strcpy is a deprecated interface (reference: https://github.com/KSPP/linux/issues/88). It should be removed from kernel source.
> It is reported as vulnerable in Enabling Linux in Safety Critical Applications (ELISA) builder.
> 
>> Why is a runtime check needed here if all of the sizes in question are known at compile time?
> Runtime check has been replaced with compile time check.
> 
> ---
> v1 -> v2: Commit message has been updated and runtime check is replace with compile time check.
> 
>   drivers/acpi/acpi_video.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c
> index 1fda30388297..be8346a66374 100644
> --- a/drivers/acpi/acpi_video.c
> +++ b/drivers/acpi/acpi_video.c
> @@ -1128,8 +1128,8 @@ static int acpi_video_bus_get_one_device(struct acpi_device *device, void *arg)
>   		return -ENOMEM;
>   	}
>   
> -	strcpy(acpi_device_name(device), ACPI_VIDEO_DEVICE_NAME);
> -	strcpy(acpi_device_class(device), ACPI_VIDEO_CLASS);
> +	strscpy(acpi_device_name(device), ACPI_VIDEO_DEVICE_NAME, sizeof(ACPI_VIDEO_DEVICE_NAME));
> +	strscpy(acpi_device_class(device), ACPI_VIDEO_CLASS, sizeof(ACPI_VIDEO_CLASS));

Hi,

for that to work, shouldn't the size of the *destination* buffer be 
passed, instead of the length of the string we want to copy?

Not tested, but the 3rd argument of strscpy () is optional.
(https://elixir.bootlin.com/linux/v6.10-rc6/source/include/linux/string.h#L87), 
so maybe just:

	strscpy(acpi_device_name(device), ACPI_VIDEO_DEVICE_NAME);

would do what you expect.

CJ

>   
>   	data->device_id = device_id;
>   	data->video = video;
> @@ -2010,8 +2010,8 @@ static int acpi_video_bus_add(struct acpi_device *device)
>   	}
>   
>   	video->device = device;
> -	strcpy(acpi_device_name(device), ACPI_VIDEO_BUS_NAME);
> -	strcpy(acpi_device_class(device), ACPI_VIDEO_CLASS);
> +	strscpy(acpi_device_name(device), ACPI_VIDEO_BUS_NAME, sizeof(ACPI_VIDEO_BUS_NAME));
> +	strscpy(acpi_device_class(device), ACPI_VIDEO_CLASS, sizeof(ACPI_VIDEO_CLASS));
>   	device->driver_data = video;
>   
>   	acpi_video_bus_find_cap(video);
diff mbox series

Patch

diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c
index 1fda30388297..be8346a66374 100644
--- a/drivers/acpi/acpi_video.c
+++ b/drivers/acpi/acpi_video.c
@@ -1128,8 +1128,8 @@  static int acpi_video_bus_get_one_device(struct acpi_device *device, void *arg)
 		return -ENOMEM;
 	}
 
-	strcpy(acpi_device_name(device), ACPI_VIDEO_DEVICE_NAME);
-	strcpy(acpi_device_class(device), ACPI_VIDEO_CLASS);
+	strscpy(acpi_device_name(device), ACPI_VIDEO_DEVICE_NAME, sizeof(ACPI_VIDEO_DEVICE_NAME));
+	strscpy(acpi_device_class(device), ACPI_VIDEO_CLASS, sizeof(ACPI_VIDEO_CLASS));
 
 	data->device_id = device_id;
 	data->video = video;
@@ -2010,8 +2010,8 @@  static int acpi_video_bus_add(struct acpi_device *device)
 	}
 
 	video->device = device;
-	strcpy(acpi_device_name(device), ACPI_VIDEO_BUS_NAME);
-	strcpy(acpi_device_class(device), ACPI_VIDEO_CLASS);
+	strscpy(acpi_device_name(device), ACPI_VIDEO_BUS_NAME, sizeof(ACPI_VIDEO_BUS_NAME));
+	strscpy(acpi_device_class(device), ACPI_VIDEO_CLASS, sizeof(ACPI_VIDEO_CLASS));
 	device->driver_data = video;
 
 	acpi_video_bus_find_cap(video);