Re: [PATCH v3 3/3] ACPI: APEI: GHES: Add NVIDIA vendor CPER record handler

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Dropping the `__packed` attribute from `struct cper_sec_nvidia` causes Undefined Behavior and potential alignment faults when accessing 64-bit fields.
- [High] A signed-to-unsigned conversion vulnerability in the length check allows a maliciously crafted ACPI CPER record to bypass bounds validation.
--

--- Patch [3]: [PATCH v3 3/3] ACPI: APEI: GHES: Add NVIDIA vendor CPER record handler ---
commit ca65b6ef5ec8eb0410ab8adcafb43315cd0cfefa
Author: Kai-Heng Feng <[email protected]>

ACPI: APEI: GHES: Add NVIDIA vendor CPER record handler

This commit adds support for decoding NVIDIA-specific CPER sections via
the APEI GHES vendor record notifier chain. It registers a notifier_block
to parse and print error metadata and diagnostic register dumps.

> diff --git a/drivers/acpi/apei/ghes-nvidia.c b/drivers/acpi/apei/ghes-nvidia.c
> new file mode 100644
> index 0000000000000..597275d81de85
> --- /dev/null
> +++ b/drivers/acpi/apei/ghes-nvidia.c
> @@ -0,0 +1,149 @@
[ ... ]
> +struct cper_sec_nvidia {
> +	char	signature[16];
> +	__le16	error_type;
> +	__le16	error_instance;
> +	u8	severity;
> +	u8	socket;
> +	u8	number_regs;
> +	u8	reserved;
> +	__le64	instance_base;
> +	struct {
> +		__le64	addr;
> +		__le64	val;
> +	} regs[] __counted_by(number_regs);
> +};

[Severity: High]
Does dropping the __packed attribute from this structure introduce a risk of
unaligned memory accesses?

The ACPI GHES error payload is often located at a 4-byte aligned offset
(such as offset 84 or 92) from the estatus base block, meaning the pointer
returned by acpi_hest_get_payload() can be unaligned.

Casting this misaligned pointer to a struct with 8-byte alignment
requirements and accessing 64-bit members like instance_base or the regs
array can result in undefined behavior or alignment faults on architectures
with strict alignment requirements.

[ ... ]
> +static int nvidia_ghes_notify(struct notifier_block *nb,
> +			      unsigned long event, void *data)
> +{
> +	struct acpi_hest_generic_data *gdata = data;
> +	struct nvidia_ghes_private *priv;
> +	const struct cper_sec_nvidia *nvidia_err;
> +	guid_t sec_guid;
> +
> +	import_guid(&sec_guid, gdata->section_type);
> +	if (!guid_equal(&sec_guid, &nvidia_sec_guid))
> +		return NOTIFY_DONE;
> +
> +	priv = container_of(nb, struct nvidia_ghes_private, nb);
> +
> +	if (acpi_hest_get_error_length(gdata) < sizeof(*nvidia_err)) {

[Severity: High]
Could a maliciously crafted ACPI CPER record bypass this length validation?

Looking at acpi_hest_get_error_length(), it returns an int. If the CPER
record contains an error_data_length larger than INT_MAX, it will be
returned as a negative int.

When this negative value is compared against sizeof(*nvidia_err), which is
a size_t, it will be promoted to a very large unsigned integer. This causes
the less-than condition to evaluate to false, bypassing the early return.

The bypassed length would then be passed as a size_t to
nvidia_ghes_print_error(), where it again evaluates the
error_data_length < min_size check as false, allowing the loop to perform
an out-of-bounds memory read.

> +		dev_err(priv->dev, "Section too small (%d < %zu)\n",
> +			acpi_hest_get_error_length(gdata), sizeof(*nvidia_err));
> +		return NOTIFY_OK;
> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3