linux-kernel - Re: [PATCH 2/4] ACPI: sysfs: Fix a potential out-of-bound write in create_of

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]

Date:   Tue, 24 Oct 2023 08:55:25 +0300
From:   Dan Carpenter <dan.carpenter@...aro.org>
To:     Christophe JAILLET <christophe.jaillet@...adoo.fr>
Cc:     rafael@...nel.org, lenb@...nel.org, linux-acpi@...r.kernel.org,
        linux-kernel@...r.kernel.org, kernel-janitors@...r.kernel.org
Subject: Re: [PATCH 2/4] ACPI: sysfs: Fix a potential out-of-bound write in
 create_of_modalias()

On Mon, Oct 23, 2023 at 09:33:16PM +0200, Christophe JAILLET wrote:
> The remaining size of the buffer used by snprintf() is not updated after
> the first write, so subsequent write in the 'for' loop a few lines below
> can write some data past the end of the 'modalias' buffer.
> 
> Correctly update the remaining size.
> 
> Note that this pattern is already correctly written in
> create_pnp_modalias().
> 
> Fixes: 8765c5ba1949 ("ACPI / scan: Rework modalias creation when "compatible" is present")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@...adoo.fr>
> ---
>  drivers/acpi/device_sysfs.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/acpi/device_sysfs.c b/drivers/acpi/device_sysfs.c
> index 4deb36dccb73..7ec3142f3eda 100644
> --- a/drivers/acpi/device_sysfs.c
> +++ b/drivers/acpi/device_sysfs.c
> @@ -215,6 +215,8 @@ static int create_of_modalias(const struct acpi_device *acpi_dev, char *modalias
>  	if (len >= size)
>  		return -ENOMEM;
>  
> +	size -= len;
> +

Yeah.  This is a good bugfix but it also shows why the canonical format
is better.  In the canonical format the "size - len" happens as part of
snprintf() instead of on a separate line where it can be forgotten.

	len += snprintf(buf + len, size - len, "string");

Also the user space version of snprintf() can fail but the
kernel space version can't.  This code is more complicated and introduces
a memory corruption bug because it is pretending that we need to check
for negatives.  People (someone) sometimes (once ten years ago) tell me
that checking for negatives is important for security but actually it's
the reverse.

regards,
dan carpenter