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@xxxxxxxxx>
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@xxxxxxxxxx>
> ---
> 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
>
