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] Use-After-Free during context restore batch buffer extraction.
- [High] Buffer over-read in `parse_wa_bb_lines` when parsing configuration strings that do not end with a trailing newline or space.
--

--- Patch [5]: [PATCH 05/10] drm/xe: Move debug configfs entries to xe_configfs_debug.c ---
commit 4db52fe672f5f434404033340d5405ac8d0e0318
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. Ensure these are wrapped in
CONFIG_DRM_XE_DEBUG to allow finer grained debug changes outside of more
ABI specific configfs entries.

> diff --git a/drivers/gpu/drm/xe/xe_configfs_debug.c b/drivers/gpu/drm/xe/xe_configfs_debug.c
> index 45617282cec54..b2667573efbed 100644
> --- a/drivers/gpu/drm/xe/xe_configfs_debug.c
> +++ b/drivers/gpu/drm/xe/xe_configfs_debug.c
> @@ -282,6 +282,23 @@ static struct xe_config_group_device *find_device(struct pci_dev *pdev)
>  	return xe_configfs_find_group_device(pdev);
>  }
>  
> +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 does dropping the config group reference
here expose the returned *cs pointer to a use-after-free?

If a concurrent rmdir on the device's configfs directory drops the last 
reference, xe_config_device_release() will free the batch buffer memory.
Since the caller (setup_configfs_mid_ctx_restore_bb() in xe_lrc.c) uses 
this pointer after the function returns, it might perform a memcpy() on 
freed memory.

[ ... ]

> +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++) {

[Severity: High]
This isn't a bug introduced by this patch, but can this loop update read out 
of bounds? 

If the inner parsing logic (like parse_hex()) advances p exactly to the 
null terminator (for example, when the input string doesn't end with a 
newline or space), the unconditional p++ increments the pointer past \0.
The loop condition *p on the next iteration would then evaluate 
out-of-bounds memory.

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