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

[Bjorn Helgaas <helgaas@kernel.org>]

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
>