Re: [PATCH 01/10] drm/xe: Guard configfs attribute reads in getters

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

Pre-existing issues:
- [High] The getters for ctx_restore batch buffers return a pointer to configfs-managed dynamically allocated memory that escapes the lock scope, leading to a Use-After-Free when the caller dereferences it.
--

--- Patch [1]: [PATCH 01/10] drm/xe: Guard configfs attribute reads in getters ---
commit 21ea5e4421c0b8eb92457fd7f08188c0ab0550d1
Author: Stuart Summers <[email protected]>

drm/xe: Guard configfs attribute reads in getters

The configfs getters read dev->config members without holding dev->lock,
so a concurrent store can tear the value being returned. The store paths
and a few of the getters already take the lock; make it consistent by
wrapping every read of dev->config in the getters with a scoped_guard(),
covering the surrounding checks that consume those values as well.

> diff --git a/drivers/gpu/drm/xe/xe_configfs.c b/drivers/gpu/drm/xe/xe_configfs.c
> index 052cce9621611..0a85043a9dd01 100644
> --- a/drivers/gpu/drm/xe/xe_configfs.c
> +++ b/drivers/gpu/drm/xe/xe_configfs.c

[ ... ]

> @@ -1308,10 +1313,12 @@ u32 xe_configfs_get_ctx_restore_mid_bb(struct pci_dev *pdev,
>  	if (!dev)
>  		return 0;
>  
> -	if (cs)
> -		*cs = dev->config.ctx_restore_mid_bb[class].cs;
> +	scoped_guard(mutex, &dev->lock) {
> +		if (cs)
> +			*cs = dev->config.ctx_restore_mid_bb[class].cs;
>  
> -	len = dev->config.ctx_restore_mid_bb[class].len;
> +		len = dev->config.ctx_restore_mid_bb[class].len;
> +	}
>  	config_group_put(&dev->group);
>  
>  	return len;

[Severity: High]
This isn't a bug introduced by this patch, but does returning this pointer
to the caller introduce a use-after-free risk?

The caller receives a pointer to configfs-managed memory, but the lock is
released and config_group_put() drops the reference before the caller uses
the pointer.

For example, in drivers/gpu/drm/xe/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);
    if (!count)
        return 0;
    ...
    memcpy(cmd, user_batch, count * sizeof(u32));

If a user concurrently removes the configfs directory, could config_group_put()
drop the final reference and free the batch buffer memory before the memcpy()
executes?

> @@ -1335,8 +1342,10 @@ u32 xe_configfs_get_ctx_restore_post_bb(struct pci_dev *pdev,
>  	if (!dev)
>  		return 0;
>  
> -	*cs = dev->config.ctx_restore_post_bb[class].cs;
> -	len = dev->config.ctx_restore_post_bb[class].len;
> +	scoped_guard(mutex, &dev->lock) {
> +		*cs = dev->config.ctx_restore_post_bb[class].cs;
> +		len = dev->config.ctx_restore_post_bb[class].len;
> +	}
>  	config_group_put(&dev->group);
>  
>  	return len;

[Severity: High]
This is a pre-existing issue, but similar to the mid_bb function above,
can this cause a use-after-free?

The pointer escapes the lock scope and the caller uses it after the final
reference might have been dropped:

drivers/gpu/drm/xe/xe_lrc.c:setup_configfs_post_ctx_restore_bb():

    count = xe_configfs_get_ctx_restore_post_bb(to_pci_dev(xe->drm.dev), hwe->class, &user_batch);
    if (!count)
        return 0;
    ...
    memcpy(cmd, user_batch, count * sizeof(u32));

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