Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Critical] accel: ethosu: missing NHCWB16 format rejection when OFM transpose is enabled --
commit aeae30d6b3e02156421eb2fa71edc03fce2b6adf Author: Rob Herring (Arm) <[email protected]> accel: ethosu: Validate OFM transpose This commit adds validation and dimension permutation for Ethos-U85 output feature map (OFM) transposition in the command stream validator. > diff --git a/drivers/accel/ethosu/ethosu_gem.c > b/drivers/accel/ethosu/ethosu_gem.c > index 04c0de8336cd1..fe6bf1a2f8cb2 100644 > --- a/drivers/accel/ethosu/ethosu_gem.c > +++ b/drivers/accel/ethosu/ethosu_gem.c [ ... ] > @@ -1000,8 +1054,19 @@ static int > ethosu_gem_cmdstream_copy_and_validate(struct drm_device *ddev, > case NPU_SET_OFM_PRECISION: > if (((param >> 6) & 0x3) > 1) > return -EINVAL; > - if (!ethosu_is_u65(edev) && (param & GENMASK(13, 11))) > - return -EINVAL; > + if (!ethosu_is_u65(edev)) { > + u32 transpose = > FIELD_GET(NPU_OFM_TRANSPOSE_MASK, param); > + > + switch (transpose) { > + case 4: > + case 5: > + return -EINVAL; > + } > + if (transpose && ((param >> 1) & 0x3) == 3) > + return -EINVAL; > + if ((param >> 14) == 2 && transpose) > + return -EINVAL; > + } > st.ofm.precision = param; > break; [Severity: Critical] Should ethosu_gem_cmdstream_copy_and_validate() also reject non-zero transpose when the activation format in NPU_SET_OFM_PRECISION is NHCWB16 (((param >> 6) & 0x3) == 1)? On Ethos-U85, OFM transposition is only valid with the linear NHWC format. If a command stream specifies both NHCWB16 (((param >> 6) & 0x3) == 1) and a non-zero transpose in NPU_SET_OFM_PRECISION: 1. feat_matrix_size() -> feat_matrix_permute() permutes x, y, and c into the transposed coordinates. 2. feat_matrix_length() -> feat_matrix_validate() checks: format = (fm->precision >> 6) & 0x3; stride_cmd = feat_matrix_base_cmd(type) + (format ? 6 : 4); if (!cmd_state_reg_is_set(st, stride_cmd)) return -EINVAL; which only requires NPU_SET_OFM_STRIDE_C to be set when format is 1, leaving NPU_SET_OFM_STRIDE_X unvalidated. 3. feat_matrix_length() then computes the region size using the NHCWB16 brick address calculation on the already-permuted x and c coordinates: case 1: //nhcwb16: element_size = BIT((fm->precision >> (ofm ? 1 : 2)) & 0x3); /* NHCWB16 accesses the complete final channel brick. */ if (check_mul_overflow(c / 16, (u64)fm->stride_c, &offset) || check_add_overflow(addr, offset, &addr) || check_mul_overflow(16 * x + 15, element_size, &offset) || check_add_overflow(addr, offset, &addr)) return U64_MAX; break; Can this cause feat_matrix_length() to underestimate info->region_size[fm->region] (for example, by permuting a large width into c while fm->stride_c is 0) and allow out-of-bounds DMA writes past the end of the GEM buffer? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=27
