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

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

[Krzysztof Wilczyński]

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.

Krzysztof Wilczyński (4):
  PCI/sysfs: Stop reporting _DSM failures as -EPERM
  PCI/sysfs: Decouple acpi_index from the optional device name element
  PCI/sysfs: Handle a malformed _DSM result in acpi_attr_is_visible()
  PCI/sysfs: Pass the device name length in UTF-16 code units

 Documentation/ABI/testing/sysfs-bus-pci | 14 ++--
 drivers/pci/pci-label.c                 | 94 ++++++++++++++++++-------
 2 files changed, 79 insertions(+), 29 deletions(-)

-- 
2.55.0

[PATCH 1/4] PCI/sysfs: Stop reporting _DSM failures as -EPERM

[Krzysztof Wilczyński]

Currently, dsm_get_label() returns the literal -1 on every failure
path.  The sysfs read path passes that value to userspace as
-EPERM.  Reading the "label" or "acpi_index" attribute on a platform
where the Device Name _DSM returns a malformed result then fails
with:

  $ 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, and the read
fails the same way for a privileged reader.  The error code points
at a cause that does not exist, and tools such as lspci report it
on every invocation.

Thus, return -ENODEV when the device has no ACPI companion, and
-EIO when the _DSM evaluation fails or returns an object that
cannot be parsed.  Other _DSM users in the tree report such
failures the same way.

The set of reads that succeed, and the bytes they return, stay
the same.  Only the error code of reads that already fail differs.

Link: https://github.com/pciutils/pciutils/issues/175
Fixes: 6058989bad05 ("PCI: Export ACPI _DSM provided firmware instance number and string name to sysfs")
Signed-off-by: Krzysztof Wilczyński <kwilczynski@kernel.org>
---
 drivers/pci/pci-label.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/pci-label.c b/drivers/pci/pci-label.c
index 0c6446519640..255e0ecffb09 100644
--- a/drivers/pci/pci-label.c
+++ b/drivers/pci/pci-label.c
@@ -160,12 +160,12 @@ static int dsm_get_label(struct device *dev, char *buf,
 	int len = 0;
 
 	if (!handle)
-		return -1;
+		return -ENODEV;
 
 	obj = acpi_evaluate_dsm(handle, &pci_acpi_dsm_guid, 0x2,
 				DSM_PCI_DEVICE_NAME, NULL);
 	if (!obj)
-		return -1;
+		return -EIO;
 
 	tmp = obj->package.elements;
 	if (obj->type == ACPI_TYPE_PACKAGE && obj->package.count == 2 &&
@@ -190,7 +190,7 @@ static int dsm_get_label(struct device *dev, char *buf,
 
 	ACPI_FREE(obj);
 
-	return len > 0 ? len : -1;
+	return len > 0 ? len : -EIO;
 }
 
 static ssize_t label_show(struct device *dev, struct device_attribute *attr,
-- 
2.55.0

[PATCH 2/4] PCI/sysfs: Decouple acpi_index from the optional device name element

[Krzysztof Wilczyński]

Currently, dsm_get_label() validates both elements of the Device Name
_DSM result in a single conditional.  The _DSM returns an ACPI package
of two elements, the instance number and the device name, where the
instance number is mandatory and the name is optional.  Firmware that
implements no name must return a NULL string for it.

Reads of "acpi_index" therefore fail whenever the name element is
malformed.  That attribute exports only the instance number, and the
two elements do not depend on each other.

Thus, validate each element only for the attribute that exports it.
So "acpi_index" now depends on the instance number alone, and "label"
reads fail with -EIO when the name element is neither a string nor a
buffer.  The package elements pointer is read only after the object
type has been checked.

On platforms with a valid instance number and a malformed name element
the "acpi_index" attribute starts returning data.  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.
That is the attribute assuming the value the firmware always
provided.

Link: https://github.com/pciutils/pciutils/issues/175
Signed-off-by: Krzysztof Wilczyński <kwilczynski@kernel.org>
---
 drivers/pci/pci-label.c | 55 +++++++++++++++++++++++++----------------
 1 file changed, 34 insertions(+), 21 deletions(-)

diff --git a/drivers/pci/pci-label.c b/drivers/pci/pci-label.c
index 255e0ecffb09..5b08f50653a3 100644
--- a/drivers/pci/pci-label.c
+++ b/drivers/pci/pci-label.c
@@ -157,7 +157,7 @@ static int dsm_get_label(struct device *dev, char *buf,
 {
 	acpi_handle handle = ACPI_HANDLE(dev);
 	union acpi_object *obj, *tmp;
-	int len = 0;
+	int len;
 
 	if (!handle)
 		return -ENODEV;
@@ -167,30 +167,43 @@ static int dsm_get_label(struct device *dev, char *buf,
 	if (!obj)
 		return -EIO;
 
-	tmp = obj->package.elements;
-	if (obj->type == ACPI_TYPE_PACKAGE && obj->package.count == 2 &&
-	    tmp[0].type == ACPI_TYPE_INTEGER &&
-	    (tmp[1].type == ACPI_TYPE_STRING ||
-	     tmp[1].type == ACPI_TYPE_BUFFER)) {
-		/*
-		 * The second string element is optional even when
-		 * this _DSM is implemented; when not implemented,
-		 * this entry must return a null string.
-		 */
-		if (attr == ACPI_ATTR_INDEX_SHOW) {
-			len = sysfs_emit(buf, "%llu\n", tmp->integer.value);
-		} else if (attr == ACPI_ATTR_LABEL_SHOW) {
-			if (tmp[1].type == ACPI_TYPE_STRING)
-				len = sysfs_emit(buf, "%s\n",
-						 tmp[1].string.pointer);
-			else if (tmp[1].type == ACPI_TYPE_BUFFER)
-				len = dsm_label_utf16s_to_utf8s(tmp + 1, buf);
-		}
+	if (obj->type != ACPI_TYPE_PACKAGE || obj->package.count != 2) {
+		len = -EIO;
+		goto out;
 	}
 
+	tmp = obj->package.elements;
+	if (tmp[0].type != ACPI_TYPE_INTEGER) {
+		len = -EIO;
+		goto out;
+	}
+
+	if (attr == ACPI_ATTR_INDEX_SHOW) {
+		len = sysfs_emit(buf, "%llu\n", tmp[0].integer.value);
+		goto out;
+	}
+
+	/*
+	 * Per PCI Firmware r3.3, sec 4.6.7, the device name is optional
+	 * even when this _DSM is implemented. When not implemented, this
+	 * entry must return a NULL string.
+	 */
+	switch (tmp[1].type) {
+	case ACPI_TYPE_STRING:
+		len = sysfs_emit(buf, "%s\n", tmp[1].string.pointer);
+		break;
+	case ACPI_TYPE_BUFFER:
+		len = dsm_label_utf16s_to_utf8s(&tmp[1], buf);
+		break;
+	default:
+		len = -EIO;
+		break;
+	}
+
+out:
 	ACPI_FREE(obj);
 
-	return len > 0 ? len : -EIO;
+	return len;
 }
 
 static ssize_t label_show(struct device *dev, struct device_attribute *attr,
-- 
2.55.0

[PATCH 3/4] PCI/sysfs: Handle a malformed _DSM result in acpi_attr_is_visible()

[Krzysztof Wilczyński]

Currently, the "label" and "acpi_index" attributes are created
whenever the _DSM function 0 bitmap advertises the Device Name
function, without evaluating that function.  The documented ABI
states that each attribute is created only if the firmware has
given a name or an instance number to the PCI device.

The bitmap says nothing about the object the function returns.
Firmware that advertises the function but returns an object that
cannot be parsed therefore produces attributes that exist and fail
every read, as in the report below.

Thus, evaluate the Device Name _DSM when deciding attribute visibility
and create each attribute only when the element it exports has a type
the read path accepts, mirroring the checks performed at read time.
The SMBIOS attribute group already follows this pattern by performing
the DMI lookup in its is_visible() callback.  The read side checks
remain in place since the two evaluations happen at different times.

This restores the behaviour from before commit 2fc59fe2ecdc ("PCI /
pci-label: treat PCI label with index 0 as valid label"), which
switched visibility from evaluating the function to checking only
the function 0 bitmap.  The check is now made for each attribute,
not once for both.

Fixes: 2fc59fe2ecdc ("PCI / pci-label: treat PCI label with index 0 as valid label")
Closes: https://github.com/pciutils/pciutils/issues/175
Signed-off-by: Krzysztof Wilczyński <kwilczynski@kernel.org>
---
 Documentation/ABI/testing/sysfs-bus-pci | 14 ++++++++---
 drivers/pci/pci-label.c                 | 33 ++++++++++++++++++++++++-
 2 files changed, 42 insertions(+), 5 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-bus-pci b/Documentation/ABI/testing/sysfs-bus-pci
index b767db2c52cb..e857b14c8faf 100644
--- a/Documentation/ABI/testing/sysfs-bus-pci
+++ b/Documentation/ABI/testing/sysfs-bus-pci
@@ -244,10 +244,16 @@ Contact:	Narendra K <narendra_k@dell.com>, linux-bugs@dell.com
 Description:
 		Reading this attribute will provide the firmware
 		given name (SMBIOS type 41 string or ACPI _DSM string) of
-		the PCI device.	The attribute will be created only
-		if the firmware	has given a name to the PCI device.
-		ACPI _DSM string name will be given priority if the
-		system firmware provides SMBIOS type 41 string also.
+		the PCI device. The attribute will be created only if the
+		firmware naming mechanism is implemented for the device:
+		a SMBIOS type 41 record with a non-empty reference
+		designation, or a Device Name _DSM that returns the name
+		as a string or a buffer. The value read is empty when the
+		firmware implements the _DSM but gives the device no name,
+		which per PCI Firmware r3.3, sec 4.6.7 the firmware reports
+		as a NULL string. ACPI _DSM string name will be given
+		priority if the system firmware provides SMBIOS type 41
+		string also.
 Users:
 		Userspace applications interested in knowing the
 		firmware assigned name of the PCI device.
diff --git a/drivers/pci/pci-label.c b/drivers/pci/pci-label.c
index 5b08f50653a3..abb941530c56 100644
--- a/drivers/pci/pci-label.c
+++ b/drivers/pci/pci-label.c
@@ -230,11 +230,42 @@ static umode_t acpi_attr_is_visible(struct kobject *kobj, struct attribute *a,
 				    int n)
 {
 	struct device *dev = kobj_to_dev(kobj);
+	union acpi_object *obj, *tmp;
+	umode_t mode = 0;
 
 	if (!device_has_acpi_name(dev))
 		return 0;
 
-	return a->mode;
+	/*
+	 * The bitmap from _DSM function 0 only advertises function 7,
+	 * and whether the returned object can be parsed is a separate
+	 * question. Evaluate it and expose each attribute only if the
+	 * element it exports has one of the types the read path
+	 * accepts, mirroring the checks in dsm_get_label().
+	 */
+	obj = acpi_evaluate_dsm(ACPI_HANDLE(dev), &pci_acpi_dsm_guid, 0x2,
+				DSM_PCI_DEVICE_NAME, NULL);
+	if (!obj)
+		return 0;
+
+	if (obj->type != ACPI_TYPE_PACKAGE || obj->package.count != 2)
+		goto out;
+
+	tmp = obj->package.elements;
+	if (tmp[0].type != ACPI_TYPE_INTEGER)
+		goto out;
+
+	if (a == &dev_attr_acpi_index.attr)
+		mode = a->mode;
+	else if (a == &dev_attr_label.attr &&
+		 (tmp[1].type == ACPI_TYPE_STRING ||
+		  tmp[1].type == ACPI_TYPE_BUFFER))
+		mode = a->mode;
+
+out:
+	ACPI_FREE(obj);
+
+	return mode;
 }
 
 const struct attribute_group pci_dev_acpi_attr_group = {
-- 
2.55.0

[PATCH 4/4] PCI/sysfs: Pass the device name length in UTF-16 code units

[Krzysztof Wilczyński]

Currently, dsm_label_utf16s_to_utf8s() passes the ACPI buffer
length to utf16s_to_utf8s() unchanged.  The Device Name _DSM may
return the device name as a buffer holding a UTF-16 string, and
the ACPI length counts bytes while the converter counts wchar_t
elements.  The converter therefore receives a count twice the
number of code units the buffer holds, and that count is its only
bound on the input.

A NUL code unit ends the conversion early, so a name that carries
one is converted correctly and the error stays hidden.  ACPICA
zeroes the entire result allocation, and the padding placed after
the buffer supplies that NUL for most lengths.  Lengths that leave
no padding, or a single byte of it, do not, and the conversion
then runs up to its own length in bytes past the end of the
allocation.  The bytes beyond the allocation are decoded into the
"label" attribute, which is world readable.

Thus, divide the buffer length by the size of wchar_t so that the
converter receives a count of code units.  The division truncates,
so the converter is never told to read more bytes than the buffer
holds, including for an odd length.

Reaching the out of bounds read needs firmware that returns the
name as a buffer and omits the terminator.

Fixes: 6058989bad05 ("PCI: Export ACPI _DSM provided firmware instance number and string name to sysfs")
Cc: stable@vger.kernel.org
Signed-off-by: Krzysztof Wilczyński <kwilczynski@kernel.org>
---
 drivers/pci/pci-label.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/pci-label.c b/drivers/pci/pci-label.c
index abb941530c56..08c461f1142d 100644
--- a/drivers/pci/pci-label.c
+++ b/drivers/pci/pci-label.c
@@ -144,7 +144,7 @@ static int dsm_label_utf16s_to_utf8s(union acpi_object *obj, char *buf)
 	int len;
 
 	len = utf16s_to_utf8s((const wchar_t *)obj->buffer.pointer,
-			      obj->buffer.length,
+			      obj->buffer.length / sizeof(wchar_t),
 			      UTF16_LITTLE_ENDIAN,
 			      buf, PAGE_SIZE - 1);
 	buf[len++] = '\n';
-- 
2.55.0

Re: [PATCH 2/4] PCI/sysfs: Decouple acpi_index from the optional device name element

[sashiko-bot]

> Currently, dsm_get_label() validates both elements of the Device Name
> _DSM result in a single conditional.  The _DSM returns an ACPI package
> of two elements, the instance number and the device name, where the
> instance number is mandatory and the name is optional.  Firmware that
> implements no name must return a NULL string for it.
> 
> Reads of "acpi_index" therefore fail whenever the name element is
> malformed.  That attribute exports only the instance number, and the
> two elements do not depend on each other.
> 
> Thus, validate each element only for the attribute that exports it.
> So "acpi_index" now depends on the instance number alone, and "label"
> reads fail with -EIO when the name element is neither a string nor a
> buffer.  The package elements pointer is read only after the object
> type has been checked.
> 
> On platforms with a valid instance number and a malformed name element
> the "acpi_index" attribute starts returning data.  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.
> That is the attribute assuming the value the firmware always
> provided.
> 
> Link: https://github.com/pciutils/pciutils/issues/175
> Signed-off-by: Krzysztof Wilczyński <kwilczynski@kernel.org>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260814070522.2327975-1-kwilczynski@kernel.org?part=2

Re: [PATCH 4/4] PCI/sysfs: Pass the device name length in UTF-16 code units

[sashiko-bot]

> Currently, dsm_label_utf16s_to_utf8s() passes the ACPI buffer
> length to utf16s_to_utf8s() unchanged.  The Device Name _DSM may
> return the device name as a buffer holding a UTF-16 string, and
> the ACPI length counts bytes while the converter counts wchar_t
> elements.  The converter therefore receives a count twice the
> number of code units the buffer holds, and that count is its only
> bound on the input.
> 
> A NUL code unit ends the conversion early, so a name that carries
> one is converted correctly and the error stays hidden.  ACPICA
> zeroes the entire result allocation, and the padding placed after
> the buffer supplies that NUL for most lengths.  Lengths that leave
> no padding, or a single byte of it, do not, and the conversion
> then runs up to its own length in bytes past the end of the
> allocation.  The bytes beyond the allocation are decoded into the
> "label" attribute, which is world readable.
> 
> Thus, divide the buffer length by the size of wchar_t so that the
> converter receives a count of code units.  The division truncates,
> so the converter is never told to read more bytes than the buffer
> holds, including for an odd length.
> 
> Reaching the out of bounds read needs firmware that returns the
> name as a buffer and omits the terminator.
> 
> Fixes: 6058989bad05 ("PCI: Export ACPI _DSM provided firmware instance number and string name to sysfs")
> Cc: stable@vger.kernel.org
> Signed-off-by: Krzysztof Wilczyński <kwilczynski@kernel.org>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260814070522.2327975-1-kwilczynski@kernel.org?part=4

Re: [PATCH 1/4] PCI/sysfs: Stop reporting _DSM failures as -EPERM

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Buffer over-read in dsm_label_utf16s_to_utf8s() due to passing length in bytes instead of 16-bit characters.
--

--- Patch [1]: [PATCH 1/4] PCI/sysfs: Stop reporting _DSM failures as -EPERM ---
commit cd414532673d45c83fe3ca965ef9476bab4c3514
Author: Krzysztof Wilczyński <kwilczynski@kernel.org>

    PCI/sysfs: Stop reporting _DSM failures as -EPERM

This patch updates dsm_get_label() to return appropriate error codes (-ENODEV
and -EIO) instead of the literal -1. This resolves incorrect -EPERM errors in
sysfs reads when ACPI companions are missing or _DSM evaluations fail.

Link: https://github.com/pciutils/pciutils/issues/175

> diff --git a/drivers/pci/pci-label.c b/drivers/pci/pci-label.c
> index 0c64465196404..255e0ecffb096 100644
> --- a/drivers/pci/pci-label.c
> +++ b/drivers/pci/pci-label.c
> @@ -160,12 +160,12 @@ static int dsm_get_label(struct device *dev, char *buf,
>  	int len = 0;
>  
>  	if (!handle)
> -		return -1;
> +		return -ENODEV;

[ ... ]

While reviewing this patch, I noticed a pre-existing issue in the same file:

drivers/pci/pci-label.c:dsm_label_utf16s_to_utf8s() {
    ...
	len = utf16s_to_utf8s((const wchar_t *)obj->buffer.pointer,
			      obj->buffer.length,
			      UTF16_LITTLE_ENDIAN,
			      buf, PAGE_SIZE - 1);
    ...
}

[Severity: High]
This isn't a bug introduced by this patch, but does this code cause a buffer
over-read?

It appears utf16s_to_utf8s() expects the maximum number of 16-bit characters
as its second argument, but it is being passed obj->buffer.length in bytes.

If a PCI device with a firmware-provided ACPI _DSM returns a
non-NUL-terminated buffer of UTF-16 characters, could this read past the end
of the allocation when a user reads the world-readable sysfs label attribute?

Should the length be divided by sizeof(wchar_t) to prevent reading out of
bounds?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260814070522.2327975-1-kwilczynski@kernel.org?part=1

Re: [PATCH 3/4] PCI/sysfs: Handle a malformed _DSM result in acpi_attr_is_visible()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] The function `dsm_label_utf16s_to_utf8s` passes the byte length of an ACPI buffer as the `inlen` parameter to `utf16s_to_utf8s`, which expects the length in `wchar_t` elements, leading to an out-of-bounds read.
--

--- Patch [3]: [PATCH 3/4] PCI/sysfs: Handle a malformed _DSM result in acpi_attr_is_visible() ---
commit 1e24bdb9829d783f4fae2c39ec3d3567d6ea73df
Author: Krzysztof Wilczyński <kwilczynski@kernel.org>

PCI/sysfs: Handle a malformed _DSM result in acpi_attr_is_visible()

This commit updates acpi_attr_is_visible() to evaluate the Device Name
_DSM when deciding attribute visibility. This ensures each attribute is
only created when the exported element has a type that the read path
accepts, preventing failing reads for malformed objects.

Closes: https://github.com/pciutils/pciutils/issues/175

> diff --git a/drivers/pci/pci-label.c b/drivers/pci/pci-label.c
> index 5b08f50653a37..abb941530c560 100644
> --- a/drivers/pci/pci-label.c
> +++ b/drivers/pci/pci-label.c

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but while reviewing how the _DSM
buffer is handled in this file, I noticed a pre-existing issue in
dsm_label_utf16s_to_utf8s():

drivers/pci/pci-label.c:dsm_label_utf16s_to_utf8s() {
	int len;

	len = utf16s_to_utf8s((const wchar_t *)obj->buffer.pointer,
			      obj->buffer.length,
			      UTF16_LITTLE_ENDIAN,
			      buf, PAGE_SIZE - 1);
	buf[len++] = '\n';

	return len;
}

Does this code pass a byte length instead of a character count?

Because utf16s_to_utf8s() expects the inlen parameter to represent the number
of 16-bit characters, passing obj->buffer.length (which is the size in bytes)
might instruct the function to process up to twice as many characters as are
actually allocated for the buffer.

Could this result in an out-of-bounds heap memory read when an unprivileged
user reads the world-readable /sys/bus/pci/devices/.../label sysfs file?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260814070522.2327975-1-kwilczynski@kernel.org?part=3

Re: [PATCH 3/4] PCI/sysfs: Handle a malformed _DSM result in acpi_attr_is_visible()

[Krzysztof Wilczyński]

Hello,

> [Severity: High]
> This isn't a bug introduced by this patch, but while reviewing how the _DSM
> buffer is handled in this file, I noticed a pre-existing issue in
> dsm_label_utf16s_to_utf8s():
> 
> drivers/pci/pci-label.c:dsm_label_utf16s_to_utf8s() {
> 	int len;
> 
> 	len = utf16s_to_utf8s((const wchar_t *)obj->buffer.pointer,
> 			      obj->buffer.length,
> 			      UTF16_LITTLE_ENDIAN,
> 			      buf, PAGE_SIZE - 1);
> 	buf[len++] = '\n';
> 
> 	return len;
> }
> 
> Does this code pass a byte length instead of a character count?
> 
> Because utf16s_to_utf8s() expects the inlen parameter to represent the number
> of 16-bit characters, passing obj->buffer.length (which is the size in bytes)
> might instruct the function to process up to twice as many characters as are
> actually allocated for the buffer.
> 
> Could this result in an out-of-bounds heap memory read when an unprivileged
> user reads the world-readable /sys/bus/pci/devices/.../label sysfs file?

The last patch of this series aims to address this issue.

Thank you!

	Krzysztof

Re: [PATCH 1/4] PCI/sysfs: Stop reporting _DSM failures as -EPERM

[Krzysztof Wilczyński]

Hello,

> [Severity: High]
> This isn't a bug introduced by this patch, but does this code cause a buffer
> over-read?
> 
> It appears utf16s_to_utf8s() expects the maximum number of 16-bit characters
> as its second argument, but it is being passed obj->buffer.length in bytes.
> 
> If a PCI device with a firmware-provided ACPI _DSM returns a
> non-NUL-terminated buffer of UTF-16 characters, could this read past the end
> of the allocation when a user reads the world-readable sysfs label attribute?
> 
> Should the length be divided by sizeof(wchar_t) to prevent reading out of
> bounds?

The last patch of this series aims to address this issue.

Thank you!

	Krzysztof

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

[Krzysztof Wilczyński]

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

Re: [PATCH 4/4] PCI/sysfs: Pass the device name length in UTF-16 code units

[Krzysztof Wilczyński]

Hello Rafael,

> Currently, dsm_label_utf16s_to_utf8s() passes the ACPI buffer
> length to utf16s_to_utf8s() unchanged.  The Device Name _DSM may
> return the device name as a buffer holding a UTF-16 string, and
> the ACPI length counts bytes while the converter counts wchar_t
> elements.  The converter therefore receives a count twice the
> number of code units the buffer holds, and that count is its only
> bound on the input.
> 
> A NUL code unit ends the conversion early, so a name that carries
> one is converted correctly and the error stays hidden.  ACPICA
> zeroes the entire result allocation, and the padding placed after
> the buffer supplies that NUL for most lengths.  Lengths that leave
> no padding, or a single byte of it, do not, and the conversion
> then runs up to its own length in bytes past the end of the
> allocation.  The bytes beyond the allocation are decoded into the
> "label" attribute, which is world readable.
> 
> Thus, divide the buffer length by the size of wchar_t so that the
> converter receives a count of code units.  The division truncates,
> so the converter is never told to read more bytes than the buffer
> holds, including for an odd length.
> 
> Reaching the out of bounds read needs firmware that returns the
> name as a buffer and omits the terminator.

[...]
>  	len = utf16s_to_utf8s((const wchar_t *)obj->buffer.pointer,
> -			      obj->buffer.length,
> +			      obj->buffer.length / sizeof(wchar_t),
>  			      UTF16_LITTLE_ENDIAN,
>  			      buf, PAGE_SIZE - 1);


While looking at this from PCI perspective, I also noticed something with
ACPI code base that we could also fix.

File drivers/acpi/device_sysfs.c, where the description_show() is adding
/sys/bus/acpi/devices/.../description sysfs attribute to each device where
this is supported/available using data from the ACPI _STR method, has the
following:

  result = utf16s_to_utf8s(
          (wchar_t *)str_obj->buffer.pointer,
          str_obj->buffer.length,
          UTF16_LITTLE_ENDIAN, buf,
          PAGE_SIZE - 1);

I believe, the above code has potentially the same issue as the one PCI
has, and which we are fixing here.  I can send a small patch to fix the
this, too, if you want.

Thank you!

        Krzysztof