Re: [PATCH 0/4] PCI/sysfs: Fix the ACPI device name attributes

Hello,

Adding Martin Mareš for visibility.

> Currently, the "label" and "acpi_index" attributes are created whenever
> the _DSM function 0 bitmap advertises the Device Name function, and
> dsm_get_label() returns the literal -1 on every failure path.  Firmware
> that advertises the function but returns an object that cannot be
> parsed therefore produces two attributes that exist and fail every read
> with -EPERM:
> 
>   $ cat /sys/bus/pci/devices/0000:00:08.3/label
>   cat: /sys/bus/pci/devices/0000:00:08.3/label: Operation not permitted
> 
> Nothing in that path performs a permission check.  Tools that read the
> attribute print the failure on every invocation, which is how this
> surfaced in a pciutils report:
> 
>   https://github.com/pciutils/pciutils/issues/175
> 
> Patch 1 returns error codes that describe what failed.  Patch 2 stops a
> malformed optional device name element from also failing "acpi_index",
> which exports the mandatory instance number and does not depend on the
> name.  Patch 3 evaluates the function when deciding visibility, so that
> an attribute exists only when the element it exports has a type the read
> path accepts.  The documented ABI already describes the behaviour patch
> 3 implements, and the SMBIOS attribute group in the same file already
> works that way.
> 
> Patch 4 corrects the length dsm_label_utf16s_to_utf8s() passes to
> utf16s_to_utf8s().  The ACPI buffer length counts bytes, while the
> converter counts wchar_t elements.  For a buffer holding no NUL code
> unit the conversion reads past the end of the object allocated by
> ACPICA.  The bytes it finds there are decoded into the world readable
> "label" attribute.
> 
> Note that patch 2 changes behaviour on platforms that have a valid
> instance number together with a malformed name element: "acpi_index"
> starts returning data there.  Because udev derives the onboard
> interface name from "acpi_index", an interface on such a platform may
> be renamed once, on the first boot after this change.  The same
> mechanism, in the other direction, is recorded in dcfa9be83866 ("ACPI /
> PCI: Fix sysfs acpi_index and label errors").
> 
> No ACPI dump accompanies the report, so the object that platform returns
> is not known.  The malformed results tested below are constructed.
> 
> Tested under QEMU with an SSDT that overrides _DSM on the ACPI companion
> of a PCI device.  Function 0 advertises the Device Name function, and
> function 7 returns a chosen object.  Nine cases, on the series and on
> v7.2-rc1, with CONFIG_KASAN=y:
> 
>   Function 7 returns                        label       acpi_index
>   ----------------------------------------  ----------  ----------
>   Package(2){1, "TESTLABEL"}                TESTLABEL   1
>   Package(2){1, ""}                         empty       1
>   Package(2){1, Buffer{"BUF", terminated}}  BUF         1
>   Package(2){1, Buffer{NUL}}                empty       1
>   Package(2){0, 0}                          absent      0
>   Package(2){"BAD", "NAME"}                 absent      absent
>   Package(2){1, Buffer(0x40)} unterminated  32 chars    1
>   valid twice, then an integer              -EIO        -EIO
>   QEMU acpi-index=7                         empty       7
> 
> On v7.2-rc1 the first four and the last behave identically, so
> compliant firmware is unaffected, including the generator QEMU uses for
> stable interface names.  Package(2){0, 0} reproduces the reported
> failure there, with both attributes returning -EPERM.
> 
> For patch 4, the unterminated buffer case on v7.2-rc1 leaked two bytes
> of adjacent memory into the attribute, and under KASAN:
> 
>   BUG: KASAN: slab-out-of-bounds in utf16s_to_utf8s+0x21f/0x250
>   Read of size 2 at addr ffff88800e77b088 by task cat/109
>   ...
>   allocated 136-byte region [ffff88800e77b000, ffff88800e77b088)
>   ...
>   acpi_ut_initialize_buffer+0xc7/0x190
>   acpi_evaluate_object+0x694/0x920
>   acpi_evaluate_dsm+0x16e/0x230
>   dsm_get_label.isra.0+0x6b/0x300
> 
> With the series applied the same case reads correctly and KASAN stays
> silent.  The two terminated buffer cases produce no splat before or
> after.  Reaching the read needs the missing terminator, not the buffer
> form as such.

Thank you!

	Krzysztof