Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [Critical] Incomplete bounds validation for multi-tile feature maps in `feat_matrix_size` allows the NPU to access memory outside the GEM buffer. - [High] The `feat_matrix_length` function does not account for the 16-channel hardware brick alignment for NHCWB16 formats, underestimating the buffer size. -- commit 5efa0071e22603105740c81590f076bdcb4fef2a Author: Rob Herring (Arm) <[email protected]> accel: ethosu: Account for feature map element size The feature map bounds calculation currently accounts for only one byte of the final element. This can leave the tail of B16, B32, and B64 elements outside the validated buffer, so the commit uses the feature map element size when calculating the final byte. > diff --git a/drivers/accel/ethosu/ethosu_gem.c > b/drivers/accel/ethosu/ethosu_gem.c > index c54496fa08f4c..bca68fab05275 100644 > --- a/drivers/accel/ethosu/ethosu_gem.c > +++ b/drivers/accel/ethosu/ethosu_gem.c > @@ -415,7 +415,7 @@ static u64 feat_matrix_length(struct ethosu_device *edev, > return U64_MAX; > } > > - if (check_add_overflow(addr, 1ULL, &offset)) > + if (check_add_overflow(addr, (u64)element_size, &offset)) [Severity: High] This isn't a bug introduced by this patch, but while the patch adds a missing single element_size to the calculation, is it sufficient for NHCWB16 formats? Looking slightly above in feat_matrix_length(): case 1: //nhcwb16: element_size = BIT((fm->precision >> (ofm ? 1 : 2)) & 0x3); if (check_mul_overflow(c / 16, (u64)fm->stride_c, &offset) || check_add_overflow(addr, offset, &addr) || check_mul_overflow(16 * x + (c & 0xf), element_size, &offset) || check_add_overflow(addr, offset, &addr)) return U64_MAX; break; The Ethos-U NPU inherently processes NHCWB16 data in 16-channel bricks. If the channel depth modulo 16 is less than 15, the hardware will still read or write the full 16-element brick. Because the validation calculation stops precisely after the specified channel and doesn't appear to round up the offset to the 16-element brick boundary, could it underestimate the required buffer size by up to 15 * element_size bytes, leading to out-of-bounds hardware memory accesses? > return U64_MAX; > > info->region_size[fm->region] = max(info->region_size[fm->region], > offset); [Severity: Critical] This is a pre-existing issue, but incomplete bounds validation for multi-tile feature maps in feat_matrix_size() might allow the NPU to access memory outside the GEM buffer. In drivers/accel/ethosu/ethosu_gem.c:feat_matrix_size(): 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); if (ret) return ret; } } return 0; } The loop evaluates the minimum and global maximum coordinates but skips the internal boundary coordinates (like fm->width0) of intermediate tiles. Similarly, for vertically split tiles (storage == 1): if (storage == 1) { ret = feat_matrix_check_location(edev, info, st, fm, type, x, 0, c, ofm, max_len); The maximum Y coordinate within the first tile (fm->height[0]) is completely omitted from bounds validation. If a multi-tile feature map is submitted, and tile 0's base address is configured near the end of the GEM allocation, with a massive internal width0 and stride_x for tile 0 (while properly bounding subsequent tiles so the global maximum falls into tile 1), could this artificially bypass validation for the rest of tile 0 and allow unconstrained memory accesses? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=11
