Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] Unvalidated hardware error severity and component values are converted to `_MAX` sentinels and used as array indices, causing out-of-bounds memory accesses. - [High] The event deduplication logic tracks only the component ID, causing distinct RAS events of different severities to be silently dropped. - [Medium] Using an unvalidated 8-bit `component` value with `BIT()` causes undefined behavior on out-of-bounds shifts and logical failures due to truncation. --
--- Patch [2]: [PATCH v6 2/2] drm/xe/xe_ras: Report correctable/uncorrectable error events to userspace --- commit 27331cfb50f32fc2a792ea10818220c4af94ef3d Author: Riana Tauro <[email protected]> drm/xe/xe_ras: Report correctable/uncorrectable error events to userspace When an interrupt is received for correctable errors indicating that error counter has crossed its threshold, read the current counter value and deliver a drm-ras error-event to userspace for each affected component. > diff --git a/drivers/gpu/drm/xe/xe_drm_ras.c b/drivers/gpu/drm/xe/xe_drm_ras.c > index 7937d8ba0ed9c..5cbfd12d61f4b 100644 > --- a/drivers/gpu/drm/xe/xe_drm_ras.c > +++ b/drivers/gpu/drm/xe/xe_drm_ras.c [ ... ] > @@ -185,6 +185,37 @@ static int register_nodes(struct xe_device *xe) > return ret; > } > > +/** > + * xe_drm_ras_event() - Report drm-ras error event to userspace > + * @xe: xe device structure > + * @component: error component (see &enum drm_xe_ras_error_component) > + * @severity: error severity (see &enum drm_xe_ras_error_severity) > + * @value: value of error counter > + * > + * Report an error-event to userspace. > + */ > +void xe_drm_ras_event(struct xe_device *xe, u8 component, u8 severity, u32 > value) > +{ > + struct xe_drm_ras *ras = &xe->ras; > + struct xe_drm_ras_counter *info = ras->info[severity]; > + struct drm_ras_node *node; > + int ret; > + > + /* Event is supported only if drm-ras is enabled */ > + if (!xe->info.has_drm_ras) > + return; > + > + node = &ras->node[severity]; > + > + if (!info || !info[component].name) > + return; [Severity: High] Does this code introduce a regression where an unexpected firmware response can cause an out-of-bounds memory access here? If the firmware returns an unsupported severity or component value, xe_to_drm_ras_severity() and xe_to_drm_ras_component() map these to DRM_XE_RAS_ERR_SEV_MAX and DRM_XE_RAS_ERR_COMP_MAX respectively. Then xe_drm_ras_event() uses these sentinel values as array indices: struct xe_drm_ras_counter *info = ras->info[severity]; ... node = &ras->node[severity]; ... if (!info || !info[component].name) This evaluates exactly one element past the end of the arrays. Should we bounds-check or reject _MAX values before using them as indices? [ ... ] > diff --git a/drivers/gpu/drm/xe/xe_ras.c b/drivers/gpu/drm/xe/xe_ras.c > index a31e06b8aa67a..d43f614de7237 100644 > --- a/drivers/gpu/drm/xe/xe_ras.c > +++ b/drivers/gpu/drm/xe/xe_ras.c [ ... ] > @@ -312,8 +364,10 @@ void xe_ras_counter_threshold_crossed(struct xe_device > *xe, > struct xe_ras_threshold_crossed *pending = (void *)&response->data; > struct xe_ras_error_class *errors = pending->counters; > u32 id, ncounters = pending->ncounters; > + u8 sent = 0; > > BUILD_BUG_ON(sizeof(response->data) < sizeof(*pending)); > + BUILD_BUG_ON(BITS_PER_TYPE(sent) < XE_RAS_COMP_MAX); > xe_device_assert_mem_access(xe); > > if (!ncounters || ncounters > XE_RAS_NUM_COUNTERS) > @@ -329,6 +383,13 @@ void xe_ras_counter_threshold_crossed(struct xe_device > *xe, > > xe_warn(xe, "[RAS]: %s %s detected\n", > comp_to_str(component), sev_to_str(severity)); > + > + /* Send event once per component */ > + if (sent & BIT(component)) > + continue; > + sent |= BIT(component); [Severity: Medium] Does this code introduce a regression if the sysctrl firmware returns an unexpected component value >= 8? The sent variable is an 8-bit u8. BIT() expands to 1UL << component, which produces a value >= 256. When assigned to the u8 sent variable, this truncates to 0, breaking deduplication. Furthermore, if the component value is >= 64, shifting by more than the width of unsigned long causes undefined behavior. [Severity: High] Does this deduplication logic introduce a regression by dropping distinct RAS events? If the hardware sysctrl returns multiple errors for the same component in a single response, but with different severities (e.g., one correctable, one uncorrectable), this condition evaluates only the component ID. The first processed event marks the component as sent, causing all subsequent events for that component to be silently dropped regardless of their severity. Should we track both component and severity to avoid losing these hardware events? > + > + ras_send_error_event(xe, severity, component); > } > } [ ... ] > @@ -425,6 +487,12 @@ enum xe_ras_recovery_action xe_ras_process_errors(struct > xe_device *xe) > xe_info(xe, "[RAS]: %s %s detected\n", > comp_to_str(component), > sev_to_str(severity)); > > + /* Send event once per component */ > + if (!(sent & BIT(component))) { > + sent |= BIT(component); > + ras_send_error_event(xe, severity, component); > + } [Severity: Medium] Does this logic exhibit the same regressions mentioned above? Since it uses the same sent bitmask logic, the truncation/undefined behavior and the distinct RAS event dropping regressions apply here as well. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
