Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [Critical] Missing lower-bound tracking for negative DMA strides allows the NPU to access memory before the start of the buffer. - [Critical] Missing feature matrix configuration parameters result in `-1` defaults that cause integer underflow in bounds checks, leading to out-of-bounds DMA. -- --- Patch [1]: [PATCH] accel/ethosu: validate all tiles and the packed-channel peak in the cmdstream verifier --- commit 272f98612406533d182249a7aefbd612bfb04a39 Author: Doruk Tan Ozturk <[email protected]> Subject: accel/ethosu: validate all tiles and the packed-channel peak in the cmdstream verifier The patch addresses an issue where feat_matrix_length() computed the maximum byte offset by evaluating a single presumed-worst corner. This allowed unprivileged processes to craft command streams that bypassed submit checks with undersized output buffers, leading to out-of-bounds DMA writes. The fix evaluates the extent at every corner that can bound the populated tiles and at the packed-channel peak. > diff --git a/drivers/accel/ethosu/ethosu_gem.c > b/drivers/accel/ethosu/ethosu_gem.c > index 3401883e207fc..dfc499d7fb0c1 100644 > --- a/drivers/accel/ethosu/ethosu_gem.c > +++ b/drivers/accel/ethosu/ethosu_gem.c > @@ -192,17 +192,19 @@ static u64 dma_length(struct > ethosu_validated_cmdstream_info *info, > return len; > } [Severity: Critical] This is a pre-existing issue, but is there a missing lower-bound check for negative DMA strides in dma_length()? When dma->stride[0] is negative, this function reduces the computed length, which allows the upper-bound check (len + offset) to pass: if (mode >= 1) { if (dma->stride[0] < 0 && (u64)(-dma->stride[0]) > len) return U64_MAX; len += dma->stride[0]; ... However, there is no lower-bound validation to ensure the minimum accessed address does not drop below 0. Could an unprivileged user submit a command stream with a negative stride and an offset near 0, causing the hardware DMA engine to drop below offset 0 and access memory preceding the GEM buffer? > > -static u64 feat_matrix_length(struct ethosu_validated_cmdstream_info *info, > - struct feat_matrix *fm, > - u32 x, u32 y, u32 c) > +/* > + * Compute the byte offset the NPU addresses for one feature-matrix > coordinate > + * (x, y, c). Storage modes 0 and 1 route disjoint (x, y) sub-rectangles to > up > + * to four independent tile bases; the caller must only pass coordinates > whose > + * tiles it means to bound. Returns U64_MAX if the coordinate lands on a tile > + * whose base was never set by the command stream. > + */ > +static u64 feat_matrix_addr(struct feat_matrix *fm, u32 x, u32 y, u32 c) > { [ ... ] > @@ -240,11 +242,66 @@ static u64 feat_matrix_length(struct > ethosu_validated_cmdstream_info *info, > break; > } > > - info->region_size[fm->region] = max(info->region_size[fm->region], addr > + 1); > - > return addr; > } [Severity: Critical] This is a pre-existing issue, but does feat_matrix_addr() handle missing configuration parameters correctly? cmd_state_init() initializes cmd_state fields to 0xff, setting unprovided s64 strides to -1. If a command stream omits a stride command, the stride remains -1. In feat_matrix_addr(), these -1 values act as subtractive strides: addr = fm->base[tile] + y * fm->stride_y; switch ((fm->precision >> 6) & 0x3) { // format case 0: //nhwc: addr += x * fm->stride_x + c; If a user provides a base offset large enough to prevent unsigned wrap-around, the maximum address evaluated in feat_matrix_length() simply remains the initial base offset at coordinate (0,0,0), completely hiding the bounds of the access. If an attacker submits a job setting a large hardware stride, and a second job omits the stride command, could the verifier use the subtractive -1 stride to pass bounds checks while the hardware performs out-of-bounds DMA using the retained large stride from the previous job? > > +/* > + * Bound the region a feature matrix touches over the coordinate box > + * [0, x] x [0, y] x [0, c]. [ ... ] -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
