Re: [PATCH 05/10] drm/xe: Move debug configfs entries to xe_configfs_debug.c

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

Pre-existing issues:
- [High] Dangling pointer returned by `xe_configfs_get_ctx_restore_mid_bb` leads to Use-After-Free during probe.
- [High] Out-of-bounds read in `parse_wa_bb_lines()` when parsing input without trailing whitespace.
--

--- Patch [5]: [PATCH 05/10] drm/xe: Move debug configfs entries to xe_configfs_debug.c ---
commit 099241979e5a310cfae46aa5d3f9fe454fd151cd
Author: Stuart Summers <[email protected]>

drm/xe: Move debug configfs entries to xe_configfs_debug.c

Move the debug specific configfs attributes into the new
xe_configfs_debug.c file under a new debug configfs subdirectory.

> diff --git a/drivers/gpu/drm/xe/xe_configfs_debug.c b/drivers/gpu/drm/xe/xe_configfs_debug.c
> --- /dev/null
> +++ b/drivers/gpu/drm/xe/xe_configfs_debug.c

[ ... ]

> +u32 xe_configfs_get_ctx_restore_mid_bb(struct pci_dev *pdev,
> +				       enum xe_engine_class class,
> +				       const u32 **cs)
> +{
> +	struct xe_config_group_device *dev = find_device(pdev);
> +	u32 len;
> +
> +	if (!dev)
> +		return 0;
> +
> +	scoped_guard(mutex, &dev->lock) {
> +		if (cs)
> +			*cs = dev->config.debug.ctx_restore_mid_bb[class].cs;
> +
> +		len = dev->config.debug.ctx_restore_mid_bb[class].len;
> +	}
> +	config_group_put(&dev->group);
> +
> +	return len;
> +}

[Severity: High]
This is a pre-existing issue, but can this lead to a use-after-free during
driver probe? The pointer is read into the cs argument while holding dev->lock,
but the lock and the config group reference are both released before the
pointer is returned to the caller. 

If a concurrent write to the configfs attribute executes
ctx_restore_mid_bb_store() and krealloc() frees the old memory, or if the
configfs directory is removed causing xe_config_device_release() to free the
memory, the caller will be left with a dangling pointer.

For example, in xe_lrc.c:setup_configfs_mid_ctx_restore_bb():
        
count = xe_configfs_get_ctx_restore_mid_bb(to_pci_dev(xe->drm.dev),
                                           hwe->class, &user_batch);
...
memcpy(cmd, user_batch, count * sizeof(u32));
        
Can we safely dereference user_batch here without holding the lock?

[ ... ]

> +static ssize_t parse_wa_bb_lines(const char *lines,
> +				 struct wa_bb wa_bb[static XE_ENGINE_CLASS_MAX])
> +{
> +	ssize_t dwords = 0, ret;
> +	const char *p;
> +
> +	for (p = lines; *p; p++) {
> +		const struct engine_info *info = NULL;
> +		u32 val, val2;
> +
> +		/* Also allow empty lines */
> +		p += strspn(p, " \t\n");
> +		if (!*p)
> +			break;
> +
> +		ret = parse_engine(p, " \t\n", NULL, &info);
> +		if (ret < 0)
> +			return ret;
> +
> +		p += ret;
> +		p += strspn(p, " \t");
> +
> +		if (str_has_prefix(p, "cmd")) {
> +			for (p += strlen("cmd"); *p;) {
> +				ret = parse_hex(p, &val);
> +				if (ret < 0)
> +					return -EINVAL;
> +				if (!ret)
> +					break;
> +
> +				p += ret;
> +				dwords++;
> +				wa_bb_append(&wa_bb[info->engine_class], val);
> +			}

[Severity: High]
This isn't a bug introduced by this patch, but does this loop read past the
end of the buffer if the input string does not end with a trailing space
or newline?

In the outer loop, the pointer p is unconditionally incremented by p++ after
each iteration. If parse_hex() processes a token without trailing whitespace
(for example, echo -n 'rcs cmd 1'), it will advance p exactly to the null
terminator. The inner loop terminates because *p is null, and execution
falls through to the end of the outer loop body. 
        
The outer loop then executes p++, advancing the pointer past the null
terminator, and the loop condition *p evaluates out-of-bounds memory.
Could this trigger a slab-out-of-bounds read?

> +		} else if (str_has_prefix(p, "reg")) {

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