[PATCH] ACPI: sysfs: Fix a buffer overrun problem with description_show()

[PATCH] ACPI: sysfs: Fix a buffer overrun problem with description_show()

[Krzysztof Wilczyński]

Currently, a device description can be obtained using ACPI, if the _STR
method exists for a particular device, and then exposed to the userspace
via a sysfs object as a string value.

If the _STR method is available for a given device then the data
(usually a Unicode string) is read and stored in a buffer (of the
ACPI_TYPE_BUFFER type) with a pointer to said buffer cached in the
struct acpi_device_pnp for later access.

The description_show() function is responsible for exposing the device
description to the userspace via a corresponding sysfs object and
internally calls the utf16s_to_utf8s() function with a pointer to the
buffer that contains the Unicode string so that it can be converted from
UTF16 encoding to UTF8 and thus allowing for the value to be safely
stored and later displayed.

When invoking the utf16s_to_utf8s() function, the description_show()
function also sets a limit of the data that can be saved into a provided
buffer as a result of the character conversion to be a total of
PAGE_SIZE, and upon completion, the utf16s_to_utf8s() function returns
an integer value denoting the number of bytes that have been written
into the provided buffer.

Following the execution of the utf16s_to_utf8s() a newline character
will be added at the end of the resulting buffer so that when the value
is read in the userspace through the sysfs object then it would include
newline making it more accessible when working with the sysfs file
system in the shell, etc.  Normally, this wouldn't be a problem, but if
the function utf16s_to_utf8s() happens to return the number of bytes
written to be precisely PAGE_SIZE, then we would overrun the buffer and
write the newline character outside the allotted space which can have
undefined consequences or result in a failure.

To fix this buffer overrun, ensure that there always is enough space
left for the newline character to be safely appended.

Fixes: d1efe3c324ea ("ACPI: Add new sysfs interface to export device description")
Signed-off-by: Krzysztof Wilczyński <kw@linux.com>
---
 drivers/acpi/device_sysfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/acpi/device_sysfs.c b/drivers/acpi/device_sysfs.c
index fa2c1c93072c..a393e0e09381 100644
--- a/drivers/acpi/device_sysfs.c
+++ b/drivers/acpi/device_sysfs.c
@@ -448,7 +448,7 @@ static ssize_t description_show(struct device *dev,
 		(wchar_t *)acpi_dev->pnp.str_obj->buffer.pointer,
 		acpi_dev->pnp.str_obj->buffer.length,
 		UTF16_LITTLE_ENDIAN, buf,
-		PAGE_SIZE);
+		PAGE_SIZE - 1);
 
 	buf[result++] = '\n';
 
-- 
2.31.1

Re: [PATCH] ACPI: sysfs: Fix a buffer overrun problem with description_show()

[Bjorn Helgaas]

On Thu, Jun 03, 2021 at 05:12:01PM +0000, Krzysztof Wilczyński wrote:
> Currently, a device description can be obtained using ACPI, if the _STR
> method exists for a particular device, and then exposed to the userspace
> via a sysfs object as a string value.
> 
> If the _STR method is available for a given device then the data
> (usually a Unicode string) is read and stored in a buffer (of the
> ACPI_TYPE_BUFFER type) with a pointer to said buffer cached in the
> struct acpi_device_pnp for later access.
> 
> The description_show() function is responsible for exposing the device
> description to the userspace via a corresponding sysfs object and
> internally calls the utf16s_to_utf8s() function with a pointer to the
> buffer that contains the Unicode string so that it can be converted from
> UTF16 encoding to UTF8 and thus allowing for the value to be safely
> stored and later displayed.
> 
> When invoking the utf16s_to_utf8s() function, the description_show()
> function also sets a limit of the data that can be saved into a provided
> buffer as a result of the character conversion to be a total of
> PAGE_SIZE, and upon completion, the utf16s_to_utf8s() function returns
> an integer value denoting the number of bytes that have been written
> into the provided buffer.
> 
> Following the execution of the utf16s_to_utf8s() a newline character
> will be added at the end of the resulting buffer so that when the value
> is read in the userspace through the sysfs object then it would include
> newline making it more accessible when working with the sysfs file
> system in the shell, etc.  Normally, this wouldn't be a problem, but if
> the function utf16s_to_utf8s() happens to return the number of bytes
> written to be precisely PAGE_SIZE, then we would overrun the buffer and
> write the newline character outside the allotted space which can have
> undefined consequences or result in a failure.
> 
> To fix this buffer overrun, ensure that there always is enough space
> left for the newline character to be safely appended.
> 
> Fixes: d1efe3c324ea ("ACPI: Add new sysfs interface to export device description")
> Signed-off-by: Krzysztof Wilczyński <kw@linux.com>

Looks right to me.  I think the critical part of the commit log is the
fact that utf16s_to_utf8s() may put up to PAGE_SIZE bytes in the
buffer, and we add a newline *after* that.

Reviewed-by: Bjorn Helgaas <bhelgaas@google.com>

> ---
>  drivers/acpi/device_sysfs.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/acpi/device_sysfs.c b/drivers/acpi/device_sysfs.c
> index fa2c1c93072c..a393e0e09381 100644
> --- a/drivers/acpi/device_sysfs.c
> +++ b/drivers/acpi/device_sysfs.c
> @@ -448,7 +448,7 @@ static ssize_t description_show(struct device *dev,
>  		(wchar_t *)acpi_dev->pnp.str_obj->buffer.pointer,
>  		acpi_dev->pnp.str_obj->buffer.length,
>  		UTF16_LITTLE_ENDIAN, buf,
> -		PAGE_SIZE);
> +		PAGE_SIZE - 1);
>  
>  	buf[result++] = '\n';
>  
> -- 
> 2.31.1
> 

Re: [PATCH] ACPI: sysfs: Fix a buffer overrun problem with description_show()

[Rafael J. Wysocki]

On Thu, Jun 3, 2021 at 7:57 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Thu, Jun 03, 2021 at 05:12:01PM +0000, Krzysztof Wilczyński wrote:
> > Currently, a device description can be obtained using ACPI, if the _STR
> > method exists for a particular device, and then exposed to the userspace
> > via a sysfs object as a string value.
> >
> > If the _STR method is available for a given device then the data
> > (usually a Unicode string) is read and stored in a buffer (of the
> > ACPI_TYPE_BUFFER type) with a pointer to said buffer cached in the
> > struct acpi_device_pnp for later access.
> >
> > The description_show() function is responsible for exposing the device
> > description to the userspace via a corresponding sysfs object and
> > internally calls the utf16s_to_utf8s() function with a pointer to the
> > buffer that contains the Unicode string so that it can be converted from
> > UTF16 encoding to UTF8 and thus allowing for the value to be safely
> > stored and later displayed.
> >
> > When invoking the utf16s_to_utf8s() function, the description_show()
> > function also sets a limit of the data that can be saved into a provided
> > buffer as a result of the character conversion to be a total of
> > PAGE_SIZE, and upon completion, the utf16s_to_utf8s() function returns
> > an integer value denoting the number of bytes that have been written
> > into the provided buffer.
> >
> > Following the execution of the utf16s_to_utf8s() a newline character
> > will be added at the end of the resulting buffer so that when the value
> > is read in the userspace through the sysfs object then it would include
> > newline making it more accessible when working with the sysfs file
> > system in the shell, etc.  Normally, this wouldn't be a problem, but if
> > the function utf16s_to_utf8s() happens to return the number of bytes
> > written to be precisely PAGE_SIZE, then we would overrun the buffer and
> > write the newline character outside the allotted space which can have
> > undefined consequences or result in a failure.
> >
> > To fix this buffer overrun, ensure that there always is enough space
> > left for the newline character to be safely appended.
> >
> > Fixes: d1efe3c324ea ("ACPI: Add new sysfs interface to export device description")
> > Signed-off-by: Krzysztof Wilczyński <kw@linux.com>
>
> Looks right to me.  I think the critical part of the commit log is the
> fact that utf16s_to_utf8s() may put up to PAGE_SIZE bytes in the
> buffer, and we add a newline *after* that.
>
> Reviewed-by: Bjorn Helgaas <bhelgaas@google.com>

Applied as 5.14 material, thanks!

> > ---
> >  drivers/acpi/device_sysfs.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/acpi/device_sysfs.c b/drivers/acpi/device_sysfs.c
> > index fa2c1c93072c..a393e0e09381 100644
> > --- a/drivers/acpi/device_sysfs.c
> > +++ b/drivers/acpi/device_sysfs.c
> > @@ -448,7 +448,7 @@ static ssize_t description_show(struct device *dev,
> >               (wchar_t *)acpi_dev->pnp.str_obj->buffer.pointer,
> >               acpi_dev->pnp.str_obj->buffer.length,
> >               UTF16_LITTLE_ENDIAN, buf,
> > -             PAGE_SIZE);
> > +             PAGE_SIZE - 1);
> >
> >       buf[result++] = '\n';
> >
> > --
> > 2.31.1
> >