1. Patchew
  2. linux
  3. View series

[PATCH] ACPI: debug: fix signedness issues in read/write helpers

Amir Mohammad Jahangirzad posted 1 patch 2 weeks, 2 days ago
There is a newer version of this series
drivers/acpi/acpi_dbg.c | 26 +++++++++++++-------------
1 file changed, 13 insertions(+), 13 deletions(-)
[PATCH] ACPI: debug: fix signedness issues in read/write helpers
Posted by Amir Mohammad Jahangirzad 2 weeks, 2 days ago 
In the ACPI debugger interface the helper functions for read and write
operations use an `int` type for the length parameter. When a large
`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
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:
- 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.


Signed-off-by: Amir Mohammad Jahangirzad <a.jahangirzad@gmail.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));
 	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));
 	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;
-- 
2.51.0
Re: [PATCH] ACPI: debug: fix signedness issues in read/write helpers
Posted by Rafael J. Wysocki 1 week, 2 days ago 
On Mon, Sep 15, 2025 at 7:21 PM Amir Mohammad Jahangirzad
<a.jahangirzad@gmail.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@gmail.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;
> --

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.