Re: [PATCH 1/1] drm/xe/configfs: Allow overriding the GuC hwconfig table

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] drm/xe: infinite loop on malformed configfs blob in
  xe_guc_hwconfig_lookup_u32()
- [Medium] drm/xe/configfs: uninitialized vmalloc memory leak in
  hwconfig_read()
--

commit 690eaa0a0c57392da54e4454901c30297777be5c
Author: Stuart Summers <stuart.summers@intel.com>

drm/xe/configfs: Allow overriding the GuC hwconfig table

This commit adds a binary configfs attribute to override the GuC hwconfig
table before binding the PCI device.

> diff --git a/drivers/gpu/drm/xe/xe_configfs.c b/drivers/gpu/drm/xe/xe_configfs.c
> index 2ddd707a6dbd3..6f810b33e12d9 100644
> --- a/drivers/gpu/drm/xe/xe_configfs.c
> +++ b/drivers/gpu/drm/xe/xe_configfs.c

[ ... ]

> @@ -937,6 +961,50 @@ static ssize_t ctx_restore_post_bb_store(struct config_item *item,
>  	return wa_bb_store(dev->config.ctx_restore_post_bb, dev, data, sz);
>  }
>  
> +static ssize_t hwconfig_read(struct config_item *item, void *buf, size_t count)
> +{
> +	struct xe_config_group_device *dev = to_xe_config_group_device(item);
> +
> +	guard(mutex)(&dev->lock);
> +
> +	if (!buf)
> +		return dev->config.hwconfig.len;
> +
> +	count = min(count, dev->config.hwconfig.len);
> +	memcpy(buf, dev->config.hwconfig.data, count);
> +
> +	return count;
> +}

[Severity: Medium]
Can a concurrent hwconfig_write() cause hwconfig_read() to leak
uninitialized vmalloc() memory to userspace?

configfs_bin_read_iter() calls ->read() twice without holding dev->lock
across both calls:

CPU0                              CPU1
configfs_bin_read_iter()
  len = hwconfig_read(..., NULL, 0)
  buffer->bin_buffer = vmalloc(len)
  buffer->bin_buffer_size = len
                                  hwconfig_write()
                                    /* replaces blob with smaller len */
  len = hwconfig_read(..., bin_buffer, len)
    /* copies only smaller len, returns >= 0 */
  copy_to_iter(bin_buffer, bin_buffer_size, to)
    <-- copies uninitialized vmalloc() tail to userspace

Because configfs_bin_read_iter() only aborts when the second ->read()
returns a negative value and otherwise copies the full bin_buffer_size,
returning a negative error (such as -EAGAIN) when count !=
dev->config.hwconfig.len would avoid the short fill.

> +
> +static ssize_t hwconfig_write(struct config_item *item, const void *buf, size_t count)
> +{
> +	struct xe_config_group_device *dev = to_xe_config_group_device(item);
> +	void *data = NULL;
> +
> +	/* The table is a sequence of dwords */
> +	if (count % sizeof(u32))
> +		return -EINVAL;

[Severity: Medium]
Can a malformed hwconfig blob written here cause an infinite loop in
xe_guc_hwconfig_lookup_u32() during device probe?

hwconfig_write() only checks that count is a multiple of 4 bytes without
validating the KLV entry lengths.  When xe_guc_hwconfig_lookup_u32()
later walks the overridden table (for example from xe_gt_mcr_init() ->
init_steering_dss() -> dss_per_group()), it advances past non-matching
keys without checking len_dw:

drivers/gpu/drm/xe/xe_guc_hwconfig.c:xe_guc_hwconfig_lookup_u32() {
    ...
	while (i + 3 <= num_dw) {
		u32 key = hwconfig[i++];
		u32 len_dw = hwconfig[i++];

		if (key != attribute) {
			i += len_dw;
			continue;
		}

		*val = hwconfig[i];
    ...
}

If an entry has len_dw == 0xfffffffe, i += len_dw wraps 32-bit int i back
to the start of the same entry and hangs the probe thread; in
xe_guc_hwconfig_dump(), i + len_dw > num_dw also wraps in 32-bit unsigned
arithmetic before widening to u64 num_dw.  In addition, if len_dw == 0 on
a matching key, xe_guc_hwconfig_lookup_u32() reads the next entry's key
dword into *val.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111000818.238585-3-stuart.summers@intel.com?part=1