linux-kernel - Re: [PATCH v2] Updating a vulnerable use of strcpy.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAJZ5v0gOKf1kv9Zj+wo5BE24ApftNrpgxuYPbZ2e_PtiqxpDJg@mail.gmail.com>
Date: Tue, 2 Jul 2024 21:28:10 +0200
From: "Rafael J. Wysocki" <rafael@...nel.org>
To: Muhammad Qasim Abdul Majeed <qasim.majeed20@...il.com>
Cc: rafael@...nel.org, lenb@...nel.org, linux-acpi@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] Updating a vulnerable use of strcpy.

On Tue, Jul 2, 2024 at 9:04 PM Muhammad Qasim Abdul Majeed
<qasim.majeed20@...il.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@...il.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);
> --