linux-kernel - Re: [PATCH] ACPI: debug: fix signedness issues in read/write helpers

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: <CAJZ5v0hsG=s37mSkcxbTqhmE_i-6skHPn+OpmCUhmJf27V+yzA@mail.gmail.com>
Date: Mon, 22 Sep 2025 19:25:28 +0200
From: "Rafael J. Wysocki" <rafael@...nel.org>
To: Amir Mohammad Jahangirzad <a.jahangirzad@...il.com>
Cc: rafael@...nel.org, lenb@...nel.org, linux-acpi@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] ACPI: debug: fix signedness issues in read/write helpers

On Mon, Sep 15, 2025 at 7:21 PM Amir Mohammad Jahangirzad
<a.jahangirzad@...il.com> wrote:
>
> In the ACPI debugger interface the helper functions for read and write
> operations use an `int` type for the length parameter. When a large

There's no need to escape data types in patch changelog and generally
please use double quotes for escaping.

> `size_t count` is passed from the file operations, this cast to `int`
> results in truncation and a negative value due to signed integer
> representation.
>
> Logically, this negative number value propagates to the `min` calculation,
> where it's selected over the positive buffer space value, leading to an
> unexpected behavior. Subsequently, when this negative value is used in
> `copy_to_user` or `copy_from_user`, it is interpreted as a large positive

Function names need not be escaped too, but please add () at the end
of each function name.

> value due to the unsigned nature of the size parameter in these functions
> causing the copy operations to attempt handling sizes far beyond the
> intended buffer limits.
>
> This patch addresses the issue by:

Please change the phrase above to "Address the issue by:"

> - Changing the length parameters in `acpi_aml_read_user` and
>   `acpi_aml_write_user` from `int` to `size_t`, aligning with the expected
>   unsigned size semantics.
> - Updating return types and local variables in acpi_aml_read() and
>   acpi_aml_write() to 'ssize_t' for consistency with kernel file operation
>   conventions.
> - Using 'size_t' for the 'n' variable to ensure calculations remain
>   unsigned.
> - Adding explicit casts to 'size_t' for circ_count_to_end() and
>   circ_space_to_end() to align types in the min() macro.

min_t() can be used instead.

>
> Signed-off-by: Amir Mohammad Jahangirzad <a.jahangirzad@...il.com>
> ---
>  drivers/acpi/acpi_dbg.c | 26 +++++++++++++-------------
>  1 file changed, 13 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/acpi/acpi_dbg.c b/drivers/acpi/acpi_dbg.c
> index d50261d05f3a1..72878840b4b75 100644
> --- a/drivers/acpi/acpi_dbg.c
> +++ b/drivers/acpi/acpi_dbg.c
> @@ -569,11 +569,11 @@ static int acpi_aml_release(struct inode *inode, struct file *file)
>         return 0;
>  }
>
> -static int acpi_aml_read_user(char __user *buf, int len)
> +static ssize_t acpi_aml_read_user(char __user *buf, size_t len)
>  {
> -       int ret;
> +       ssize_t ret;
>         struct circ_buf *crc = &acpi_aml_io.out_crc;
> -       int n;
> +       size_t n;
>         char *p;
>
>         ret = acpi_aml_lock_read(crc, ACPI_AML_OUT_USER);
> @@ -582,7 +582,7 @@ static int acpi_aml_read_user(char __user *buf, int len)
>         /* sync head before removing logs */
>         smp_rmb();
>         p = &crc->buf[crc->tail];
> -       n = min(len, circ_count_to_end(crc));
> +       n = min(len, (size_t)circ_count_to_end(crc));

Use min_t() here.

>         if (copy_to_user(buf, p, n)) {
>                 ret = -EFAULT;
>                 goto out;
> @@ -599,8 +599,8 @@ static int acpi_aml_read_user(char __user *buf, int len)
>  static ssize_t acpi_aml_read(struct file *file, char __user *buf,
>                              size_t count, loff_t *ppos)
>  {
> -       int ret = 0;
> -       int size = 0;
> +       ssize_t ret = 0;
> +       ssize_t size = 0;
>
>         if (!count)
>                 return 0;
> @@ -639,11 +639,11 @@ static ssize_t acpi_aml_read(struct file *file, char __user *buf,
>         return size > 0 ? size : ret;
>  }
>
> -static int acpi_aml_write_user(const char __user *buf, int len)
> +static ssize_t acpi_aml_write_user(const char __user *buf, size_t len)
>  {
> -       int ret;
> +       ssize_t ret;
>         struct circ_buf *crc = &acpi_aml_io.in_crc;
> -       int n;
> +       size_t n;
>         char *p;
>
>         ret = acpi_aml_lock_write(crc, ACPI_AML_IN_USER);
> @@ -652,7 +652,7 @@ static int acpi_aml_write_user(const char __user *buf, int len)
>         /* sync tail before inserting cmds */
>         smp_mb();
>         p = &crc->buf[crc->head];
> -       n = min(len, circ_space_to_end(crc));
> +       n = min(len, (size_t)circ_space_to_end(crc));

And here.

>         if (copy_from_user(p, buf, n)) {
>                 ret = -EFAULT;
>                 goto out;
> @@ -663,14 +663,14 @@ static int acpi_aml_write_user(const char __user *buf, int len)
>         ret = n;
>  out:
>         acpi_aml_unlock_fifo(ACPI_AML_IN_USER, ret >= 0);
> -       return n;
> +       return ret;
>  }
>
>  static ssize_t acpi_aml_write(struct file *file, const char __user *buf,
>                               size_t count, loff_t *ppos)
>  {
> -       int ret = 0;
> -       int size = 0;
> +       ssize_t ret = 0;
> +       ssize_t size = 0;
>
>         if (!count)
>                 return 0;
> --