Re: [PATCH 02/10] drm/xe: Sort xe_config_device fields

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Use-After-Free due to dangling pointer returned by xe_configfs_get_ctx_restore_mid_bb and post_bb escaping the reference lifetime.
- [High] Out-of-bounds Read in parse_wa_bb_lines when input lacks a trailing newline.
--

--- Patch [2]: [PATCH 02/10] drm/xe: Sort xe_config_device fields ---
commit 299f84549303f530b903f33ec9224a2a497c7599
Author: Stuart Summers <stuart.summers@intel.com>

drm/xe: Sort xe_config_device fields

As we're planning more configfs entries over time, let's add
some sorting rules to make sure this stays clean and easy to
manage. Sorting first by type and then name for readability.

> diff --git a/drivers/gpu/drm/xe/xe_configfs.c b/drivers/gpu/drm/xe/xe_configfs.c
> index 0a85043a9dd01..cb6ab1fcb8c18 100644
> --- a/drivers/gpu/drm/xe/xe_configfs.c
> +++ b/drivers/gpu/drm/xe/xe_configfs.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 = to_xe_config_group_device(item);
> -	bool survivability_mode;
> -	int ret;
> +	struct xe_config_group_device *dev = find_xe_config_group_device(pdev);
> +	u32 len;
>  
> -	ret = kstrtobool(page, &survivability_mode);
> -	if (ret)
> -		return ret;
> +	if (!dev)
> +		return 0;
>  
> -	guard(mutex)(&dev->lock);
> -	if (is_bound(dev))
> -		return -EBUSY;
> +	scoped_guard(mutex, &dev->lock) {
> +		if (cs)
> +			*cs = dev->config.ctx_restore_mid_bb[class].cs;
>  
> -	dev->config.survivability_mode = survivability_mode;
> +		len = dev->config.ctx_restore_mid_bb[class].len;
> +	}
> +	config_group_put(&dev->group);
>  
>  	return len;
>  }
>  
> -static ssize_t gt_types_allowed_show(struct config_item *item, char *page)
> +/**
> + * xe_configfs_get_ctx_restore_post_bb - get configfs ctx_restore_post_bb setting
> + * @pdev: pci device
> + * @class: hw engine class
> + * @cs: pointer to the bb to use - only valid during probe
> + *
> + * Return: Number of dwords used in the post_ctx_restore setting in configfs
> + */
> +u32 xe_configfs_get_ctx_restore_post_bb(struct pci_dev *pdev,
> +					enum xe_engine_class class,
> +					const u32 **cs)
>  {
> -	struct xe_config_device *dev = to_xe_config_device(item);
> -	char *p = page;
> -
> -	for (size_t i = 0; i < ARRAY_SIZE(gt_types); i++)
> -		if (dev->gt_types_allowed & BIT_ULL(gt_types[i].type))
> -			p += sprintf(p, "%s\n", gt_types[i].name);
> +	struct xe_config_group_device *dev = find_xe_config_group_device(pdev);
> +	u32 len;
>  
> -	return p - page;
> -}
> +	if (!dev)
> +		return 0;
>  
> -static ssize_t gt_types_allowed_store(struct config_item *item, const char *page,
> -				      size_t len)
> -{
> -	struct xe_config_group_device *dev = to_xe_config_group_device(item);
> -	char *buf __free(kfree) = kstrdup(page, GFP_KERNEL);
> -	char *p = buf;
> -	u64 typemask = 0;
> -
> -	if (!buf)
> -		return -ENOMEM;
> -
> -	while (p) {
> -		char *typename = strsep(&p, ",\n");
> -		bool matched = false;
> -
> -		if (typename[0] == '\0')
> -			continue;
> -
> -		for (size_t i = 0; i < ARRAY_SIZE(gt_types); i++) {
> -			if (strcmp(typename, gt_types[i].name) == 0) {
> -				typemask |= BIT(gt_types[i].type);
> -				matched = true;
> -				break;
> -			}
> -		}
> -
> -		if (!matched)
> -			return -EINVAL;
> -	}
> -
> -	guard(mutex)(&dev->lock);
> -	if (is_bound(dev))
> -		return -EBUSY;
> -
> -	dev->config.gt_types_allowed = typemask;
> -
> -	return len;
> -}
> -
> -static ssize_t engines_allowed_show(struct config_item *item, char *page)
> -{
> -	struct xe_config_device *dev = to_xe_config_device(item);
> -	char *p = page;
> -
> -	for (size_t i = 0; i < ARRAY_SIZE(engine_info); i++) {
> -		u64 mask = engine_info[i].mask;
> -
> -		if ((dev->engines_allowed & mask) == mask) {
> -			p += sprintf(p, "%s*\n", engine_info[i].cls);
> -		} else if (mask & dev->engines_allowed) {
> -			u16 bit0 = __ffs64(mask), bit;
> -
> -			mask &= dev->engines_allowed;
> -
> -			for_each_set_bit(bit, (const unsigned long *)&mask, 64)
> -				p += sprintf(p, "%s%u\n", engine_info[i].cls,
> -					     bit - bit0);
> -		}
> -	}
> -
> -	return p - page;
> -}
> -
> -/*
> - * Lookup engine_info. If @mask is not NULL, reduce the mask according to the
> - * instance in @pattern.
> - *
> - * Examples of inputs:
> - * - lookup_engine_info("rcs0", &mask): return "rcs" entry from @engine_info and
> - *   mask == BIT_ULL(XE_HW_ENGINE_RCS0)
> - * - lookup_engine_info("rcs*", &mask): return "rcs" entry from @engine_info and
> - *   mask == XE_HW_ENGINE_RCS_MASK
> - * - lookup_engine_info("rcs", NULL): return "rcs" entry from @engine_info
> - */
> -static const struct engine_info *lookup_engine_info(const char *pattern, u64 *mask)
> -{
> -	for (size_t i = 0; i < ARRAY_SIZE(engine_info); i++) {
> -		u8 instance;
> -		u16 bit;
> -
> -		if (!str_has_prefix(pattern, engine_info[i].cls))
> -			continue;
> -
> -		pattern += strlen(engine_info[i].cls);
> -		if (!mask)
> -			return *pattern ? NULL : &engine_info[i];
> -
> -		if (!strcmp(pattern, "*")) {
> -			*mask = engine_info[i].mask;
> -			return &engine_info[i];
> -		}
> -
> -		if (kstrtou8(pattern, 10, &instance))
> -			return NULL;
> -
> -		bit = __ffs64(engine_info[i].mask) + instance;
> -		if (bit >= fls64(engine_info[i].mask))
> -			return NULL;
> -
> -		*mask = BIT_ULL(bit);
> -		return &engine_info[i];
> -	}
> -
> -	return NULL;
> -}
> -
> -static int parse_engine(const char *s, const char *end_chars, u64 *mask,
> -			const struct engine_info **pinfo)
> -{
> -	char buf[MAX_ENGINE_CLASS_CHARS + MAX_ENGINE_INSTANCE_CHARS + 1];
> -	const struct engine_info *info;
> -	size_t len;
> -
> -	len = strcspn(s, end_chars);
> -	if (len >= sizeof(buf))
> -		return -EINVAL;
> -
> -	memcpy(buf, s, len);
> -	buf[len] = '\0';
> -
> -	info = lookup_engine_info(buf, mask);
> -	if (!info)
> -		return -ENOENT;
> -
> -	if (pinfo)
> -		*pinfo = info;
> -
> -	return len;
> -}
> -
> -static ssize_t engines_allowed_store(struct config_item *item, const char *page,
> -				     size_t len)
> -{
> -	struct xe_config_group_device *dev = to_xe_config_group_device(item);
> -	ssize_t patternlen, p;
> -	u64 mask, val = 0;
> -
> -	for (p = 0; p < len; p += patternlen + 1) {
> -		patternlen = parse_engine(page + p, ",\n", &mask, NULL);
> -		if (patternlen < 0)
> -			return -EINVAL;
> -
> -		val |= mask;
> +	scoped_guard(mutex, &dev->lock) {
> +		*cs = dev->config.ctx_restore_post_bb[class].cs;
> +		len = dev->config.ctx_restore_post_bb[class].len;
>  	}
> -
> -	guard(mutex)(&dev->lock);
> -	if (is_bound(dev))
> -		return -EBUSY;
> -
> -	dev->config.engines_allowed = val;
> -
> -	return len;
> -}
> -
> -static ssize_t enable_psmi_show(struct config_item *item, char *page)
> -{
> -	struct xe_config_device *dev = to_xe_config_device(item);
> -
> -	return sprintf(page, "%d\n", dev->enable_psmi);
> -}
> -
> -static ssize_t enable_psmi_store(struct config_item *item, const char *page, size_t len)
> -{
> -	struct xe_config_group_device *dev = to_xe_config_group_device(item);
> -	bool val;
> -	int ret;
> -
> -	ret = kstrtobool(page, &val);
> -	if (ret)
> -		return ret;
> -
> -	guard(mutex)(&dev->lock);
> -	if (is_bound(dev))
> -		return -EBUSY;
> -
> -	dev->config.enable_psmi = val;
> -
> -	return len;
> -}
> -
> -static ssize_t enable_multi_queue_show(struct config_item *item, char *page)
> -{
> -	struct xe_config_device *dev = to_xe_config_device(item);
> -
> -	return sprintf(page, "%d\n", dev->enable_multi_queue);
> -}
> -
> -static ssize_t enable_multi_queue_store(struct config_item *item, const char *page,
> -					size_t len)
> -{
> -	struct xe_config_group_device *dev = to_xe_config_group_device(item);
> -	bool val;
> -	int ret;
> -
> -	ret = kstrtobool(page, &val);
> -	if (ret)
> -		return ret;
> -
> -	guard(mutex)(&dev->lock);
> -	if (is_bound(dev))
> -		return -EBUSY;
> -
> -	dev->config.enable_multi_queue = val;
> +	config_group_put(&dev->group);
>  
>  	return len;
>  }

[Severity: High]
This is a pre-existing issue, but is it safe for xe_configfs_get_ctx_restore_mid_bb
and xe_configfs_get_ctx_restore_post_bb to return internal pointers from the
configfs context out to the caller while simultaneously dropping the reference
with config_group_put()?

Because the pointer is returned directly and the group reference is dropped,
the pointer escapes the reference lifetime. If userspace concurrently removes
the configfs directory, xe_config_device_release() will free the internal
buffer.

Could this lead to a use-after-free read during engine initialization if the
freed memory is reused before the caller is finished with the batch buffer?

[ ... ]

> +static ssize_t wa_bb_store(struct wa_bb wa_bb[static XE_ENGINE_CLASS_MAX],
> +			   struct xe_config_group_device *dev,
> +			   const char *page, size_t len)
> +{
> +	struct wa_bb tmp_wa_bb[XE_ENGINE_CLASS_MAX] = {};
> +	enum xe_engine_class class;
> +	ssize_t count;
> +	u32 *tmp;
> +
> +	/* 1. Ensure the inputs are valid and parseable */
> +	count = parse_wa_bb_lines(page, tmp_wa_bb);
> +	if (count < 0)
>  		return count;
>  
>  	guard(mutex)(&dev->lock);
> -
> +
> +	if (is_bound(dev))
> +		return -EBUSY;
> +
> +	/*
> +	 * 2. Allocate a u32 array and set the pointers to the right positions
> +	 * according to the length of each class' wa_bb
> +	 */
> +	tmp = krealloc(wa_bb[0].cs, count * sizeof(u32), GFP_KERNEL);
> +	if (!tmp)
> +		return -ENOMEM;
> +
> +	if (!count) {
> +		memset(wa_bb, 0, sizeof(tmp_wa_bb));
> +		return len;
> +	}
> +
> +	for (class = 0, count = 0; class < XE_ENGINE_CLASS_MAX; ++class) {
> +		tmp_wa_bb[class].cs = tmp + count;
> +		count += tmp_wa_bb[class].len;
> +		tmp_wa_bb[class].len = 0;
> +	}
> +
> +	/* 3. Parse wa_bb lines again, this time saving the values */
> +	count = parse_wa_bb_lines(page, tmp_wa_bb);
> +	if (count < 0)
> +		return count;

[Severity: High]
This is a pre-existing issue, but does parse_wa_bb_lines() handle configfs
inputs that lack a trailing newline safely?

If a user writes a batch buffer command to configfs without a trailing newline,
parse_hex() parses the last parameter and leaves the pointer exactly at the
null terminator.

At the end of the iteration, the outer for-loop in parse_wa_bb_lines():

	for (p = lines; *p; p++) {

unconditionally increments the pointer. This causes the pointer to jump past
the null terminator and read into uninitialized memory on the next loop
condition check.

Can this result in an out-of-bounds read and a potential kernel panic if the
read crosses a page boundary into unmapped memory?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260820210639.107524-12-stuart.summers@intel.com?part=2