Re: [PATCH v2 04/11] drm/xe/cper: APIs to prepare and log CPER record

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Unconditional dereference of potentially NULL `THIS_MODULE->srcversion` causes kernel panic.
- [Medium] Assigning `ktime_get_real_seconds()` to `rhdr->timestamp` violates the UEFI CPER standard, which requires a BCD timestamp.
--

commit 1a8a9f568395a2ce2ede1bcececcf688f5a25b63
Author: Badal Nilawar <badal.nilawar@intel.com>

drm/xe/cper: APIs to prepare and log CPER record

Add APIs to initialize Intel-specific CPER metadata, build a non-standard
CPER record, and emit it via the xe_error_cper tracepoint.

> diff --git a/drivers/gpu/drm/xe/xe_cper.c b/drivers/gpu/drm/xe/xe_cper.c
> new file mode 100644
> index 0000000000000..371537052de35
> --- /dev/null
> +++ b/drivers/gpu/drm/xe/xe_cper.c
[ ... ]
> +void xe_cper_init_intel_err_hdr(struct xe_device *xe, const u8 location[12],
> +				u64 first_timestamp, u32 sig_id,
> +				u32 error_count, struct xe_cper_sec_intel_err_hdr *ihdr)
> +{
[ ... ]
> +	strscpy(ihdr->pci_bdf, pci_name(to_pci_dev(xe->drm.dev)), sizeof(ihdr->pci_bdf));
> +	ihdr->valid_bits.pci_bdf = 1;
> +
> +#ifdef MODULE
> +	strscpy(ihdr->drv_version, THIS_MODULE->srcversion, sizeof(ihdr->drv_version));

[Severity: High]
Can this cause a kernel panic if the module's source version is not populated?

When the kernel is compiled without CONFIG_MODULE_SRCVERSION_ALL and the
module lacks an explicit MODULE_VERSION macro, the build system does not
generate the srcversion modinfo attribute. In this case,
THIS_MODULE->srcversion is left as NULL during module load.

Since strscpy() does not handle NULL source pointers gracefully, this
unconditional dereference of THIS_MODULE->srcversion in
xe_cper_init_intel_err_hdr() could lead to a kernel panic when hardware
error reporting is triggered.

> +	ihdr->valid_bits.drv_version = 1;
> +#else
> +	ihdr->valid_bits.drv_version = 0;
> +#endif
> +
> +	fill_fw_id(xe, ihdr);
> +}
[ ... ]
> +void xe_cper_record_emit(struct xe_device *xe, u8 severity,
> +			 guid_t *notification_type,
> +			 struct xe_cper_sec_intel_err_hdr *ihdr,
> +			 const void *einfo, u32 einfo_len)
> +{
[ ... ]
> +	/* Assemble the CPER record header (UEFI Appendix N.2.1) */
> +	memcpy(rhdr->signature, CPER_SIG_RECORD, CPER_SIG_SIZE);
> +	rhdr->revision          = CPER_RECORD_REV;
> +	rhdr->signature_end     = CPER_SIG_END;
> +	rhdr->section_count     = 1;
> +	rhdr->error_severity    = severity;
> +	rhdr->validation_bits   = CPER_VALID_TIMESTAMP;
> +	rhdr->record_length     = total_len;
> +	rhdr->timestamp         = ktime_get_real_seconds();

[Severity: Medium]
Does this assign a raw binary Unix timestamp to a field that requires a BCD
format?

ktime_get_real_seconds() returns a 64-bit binary integer representing
seconds since the Unix epoch. However, the UEFI standard (UEFI 2.8+
Appendix N.2.1) mandates that the timestamp field in a struct
cper_record_header is a 64-bit BCD-encoded date and time value (Century,
Year, Month, Day, Hours, Minutes, Seconds).

Assigning a raw binary Unix timestamp to rhdr->timestamp in
xe_cper_record_emit() could cause standard CPER parsers to read invalid
BCD characters or absurd dates.

> +	if (platform_id) {
> +		rhdr->platform_id      = *platform_id;
> +		rhdr->validation_bits |= CPER_VALID_PLATFORM_ID;
> +	}
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260825175916.1103841-13-badal.nilawar@intel.com?part=4