Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues: - [Medium] The driver violates the DRM RAS framework API contract by failing to perform bounds checking on the user-supplied threshold value. Pre-existing issues: - [High] Lack of reference counting or RCU read locks in the generic drm_ras core causes a TOCTOU race leading to Use-After-Free of drm_ras_node and xe_device during concurrent device unbind. -- --- Patch [4]: [PATCH v5 4/5] drm/xe/drm_ras: Wire up error threshold callbacks --- commit a57e8de665a45c38e3544fb21f325ce074bc995d Author: Raag Jadav <[email protected]> drm/xe/drm_ras: Wire up error threshold callbacks This commit wires up the get/set error threshold support in the xe driver to the generic drm_ras core. This allows userspace to query and configure correctable error thresholds via Netlink. > diff --git a/drivers/gpu/drm/xe/xe_drm_ras.c b/drivers/gpu/drm/xe/xe_drm_ras.c > --- a/drivers/gpu/drm/xe/xe_drm_ras.c > +++ b/drivers/gpu/drm/xe/xe_drm_ras.c > @@ -86,6 +86,38 @@ static int clear_correctable_error_counter(struct > drm_ras_node *node, u32 error_ > return clear_error_counter(xe, DRM_XE_RAS_ERR_SEV_CORRECTABLE, > error_id); > } > > +static int query_correctable_error_threshold(struct drm_ras_node *ep, u32 > error_id, > + const char **name, u32 *threshold) > +{ > + struct xe_device *xe = ep->priv; [Severity: High] This is a pre-existing issue, but can this dereference of ep and ep->priv lead to a use-after-free during concurrent device unbind? If a Netlink request calls get_node_error_threshold() in the generic drm_ras core, it retrieves the node via xa_load() but doesn't hold RCU or take a reference count. Concurrently, if the device is unbound, the drmm action cleanup_node() could free the drm_ras_node and the xe_device. When the Netlink thread resumes and calls this callback, could the ep pointer and the xe_device it points to already be freed? > + struct xe_drm_ras *ras = &xe->ras; > + struct xe_drm_ras_counter *info = > ras->info[DRM_XE_RAS_ERR_SEV_CORRECTABLE]; [ ... ] > +static int set_correctable_error_threshold(struct drm_ras_node *ep, u32 > error_id, u32 threshold) > +{ > + struct xe_device *xe = ep->priv; > + struct xe_drm_ras *ras = &xe->ras; > + struct xe_drm_ras_counter *info = > ras->info[DRM_XE_RAS_ERR_SEV_CORRECTABLE]; > + > + if (!info || !info[error_id].name) > + return -ENOENT; > + > + if (!xe->info.has_sysctrl) > + return -EOPNOTSUPP; > + > + return xe_ras_set_threshold(xe, DRM_XE_RAS_ERR_SEV_CORRECTABLE, > error_id, threshold); [Severity: Medium] Is this missing a bounds check on the user-supplied threshold value? The DRM RAS documentation in include/drm/drm_ras.h explicitly mandates that the driver is responsible for error threshold bounds checking in the set_error_threshold() callback. Here, the threshold value is passed directly to xe_ras_set_threshold() and the system controller without validation. Could this lead to speculative hardware misbehavior or interrupt storms if userspace passes malformed values like 0 or an extremely large integer? > +} > + > static struct xe_drm_ras_counter *allocate_and_copy_counters(struct > xe_device *xe) -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=4
