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] [day] [month] [year] [list]

Message-ID: <8f2284e3-bf6b-44ff-b34f-9512b1155542@wanadoo.fr>
Date: Tue, 2 Jul 2024 21:29:31 +0200
From: Christophe JAILLET <christophe.jaillet@...adoo.fr>
To: Muhammad Qasim Abdul Majeed <qasim.majeed20@...il.com>,
 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.

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

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