Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [Critical] feat_matrix_size completely fails to evaluate intermediate tile boundaries, allowing arbitrary out-of-bounds DMA access - [High] feat_matrix_length assumes monotonic address growth for nhcwb16 format, underestimating buffer bounds by up to 60 bytes -- commit 5cfa238ea5bc9f37e1c0be661f6a282b416aafb4 Author: Rob Herring (Arm) <[email protected]> Subject: accel: ethosu: Validate OFM transpose U85 OFM dimensions are specified before transposition, while tile bases and strides address the transposed feature map. Permute the output endpoint before validating its tile and stride accesses. > diff --git a/drivers/accel/ethosu/ethosu_gem.c > b/drivers/accel/ethosu/ethosu_gem.c > index 559fbf55f12d5..8114447891b25 100644 > --- a/drivers/accel/ethosu/ethosu_gem.c > +++ b/drivers/accel/ethosu/ethosu_gem.c > @@ -397,6 +397,52 @@ static int feat_matrix_validate(struct ethosu_device > *edev, > > return 0; > } > + > +static int feat_matrix_permute(struct ethosu_device *edev, > + struct feat_matrix *fm, u32 *x, u32 *y, > + u32 *c, bool ofm) > +{ [ ... ] > + default: > + return -EINVAL; > + } > + > + return 0; > +} > static u64 feat_matrix_length(struct ethosu_device *edev, > struct ethosu_validated_cmdstream_info *info, > struct cmd_state *st, struct feat_matrix *fm, [Severity: High] This isn't a bug introduced by this patch, but feat_matrix_length() assumes monotonic address growth for the nhcwb16 format, underestimating buffer bounds by up to 60 bytes. For the nhcwb16 format, the address offset calculation: check_mul_overflow(c / 16, (u64)fm->stride_c, &offset) || ... check_mul_overflow(16 * x + (c & 0xf), element_size, &offset) is not monotonic with respect to c if stride_c is smaller than 15 * element_size (which userspace fully controls). Because the validator only checks the offset at the maximum depth, malicious userspace can set stride_c to 0 and c_max to 16. It then evaluates only at c=16, which yields 16 * x * element_size. However, the hardware will also access c=15, resulting in a larger offset of 16 * x * element_size + 15 * element_size. This allows out-of-bounds DMA access into adjacent kernel memory. Can this logic be updated to evaluate the maximum offset accurately by accounting for intermediate values of c? > @@ -502,6 +548,9 @@ static int feat_matrix_size(struct ethosu_device *edev, > int ret; > > *max_len = 0; > + ret = feat_matrix_permute(edev, fm, &x, &y, &c, ofm); > + if (ret) > + return ret; > > if (ethosu_is_u65(edev) || storage == 0) { > for (int xi = 0; xi < 2; xi++) { > for (int yi = 0; yi < 2; yi++) { > ret = feat_matrix_check_location(edev, info, > st, fm, type, > xi ? x : 0, > yi ? y : 0, c, > ofm, > max_len); [Severity: Critical] This is a pre-existing issue, but feat_matrix_size() completely fails to evaluate intermediate tile boundaries, allowing arbitrary out-of-bounds DMA access. For tiled matrices (storage == 0 or storage == 1), each tile has an independent base address. The maximum memory offset within tile 0 occurs at x = min(x, fm->width0) and y = min(y, fm->height[0]). The validator never evaluates these intermediate maxima: For storage == 0, it only evaluates x at 0 and max_x in the loop: for (int xi = 0; xi < 2; xi++) { for (int yi = 0; yi < 2; yi++) { ret = feat_matrix_check_location(..., xi ? x : 0, yi ? y : 0, ...); This misses the local maximum x offset inside tile 0 (fm->width0). For storage == 1, it evaluates y at 0 and fm->height[1]: if (fm->height[0] < fm->height[1] && fm->height[1] <= y) { ret = feat_matrix_check_location(..., x, fm->height[1], ...); This completely skips the end of tile 0 (fm->height[0]), leaving the max offset for tile 0 unvalidated. Userspace can bypass validation by setting base[0] near the end of the buffer while base[0] + offset(x, fm->height[0]) massively overflows it. Could this be corrected to evaluate the local maxima for tile boundaries? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=18
