On Thu, Jun 3, 2021 at 7:57 PM Bjorn Helgaas <helgaas@xxxxxxxxxx> 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@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>
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
> >
