[v2] Updating a vulnerable use of strcpy.

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

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

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