Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues: - [Critical] The command stream validation in `feat_matrix_size` fails to check the maximum accessed address within intermediate tiles, allowing an attacker to bypass bounds checks and cause the NPU to access out-of-bounds memory. -- commit eae7ad151b58329b8dda3932561e68aa3827908a Author: Rob Herring (Arm) <[email protected]> accel: ethosu: Account for kernel dilation in IFM size Kernel dilation increases the input feature-map area accessed by a kernel operation. Include the x and y dilation settings when calculating the required IFM bounds. Without this, a command stream using x2 dilation can access IFM memory past the range validated by the driver. > diff --git a/drivers/accel/ethosu/ethosu_device.h > b/drivers/accel/ethosu/ethosu_device.h > index d7e1e3c8ca12d..8e23fdbf7f8ab 100644 > --- a/drivers/accel/ethosu/ethosu_device.h > +++ b/drivers/accel/ethosu/ethosu_device.h [ ... ] > diff --git a/drivers/accel/ethosu/ethosu_gem.c > b/drivers/accel/ethosu/ethosu_gem.c > index d60a2a453302b..63dd07791f314 100644 > --- a/drivers/accel/ethosu/ethosu_gem.c > +++ b/drivers/accel/ethosu/ethosu_gem.c [ ... ] > @@ -536,10 +536,16 @@ static int calc_sizes(struct drm_device *ddev, > ((st->ifm.stride_kernel >> 1) & 0x1) + 1; > u32 stride_x = ((st->ifm.stride_kernel >> 5) & 0x2) + > (st->ifm.stride_kernel & 0x1) + 1; > + u32 dilation_y = 1 + !!(st->ifm.stride_kernel & > + NPU_KERNEL_DILATION_Y); > + u32 dilation_x = 1 + !!(st->ifm.stride_kernel & > + NPU_KERNEL_DILATION_X); > s32 ifm_height = st->ofm.height[2] * stride_y + > - st->ifm.height[2] - (st->ifm.pad_top + > st->ifm.pad_bottom); > + st->ifm.height[2] * dilation_y - > + (st->ifm.pad_top + st->ifm.pad_bottom); > s32 ifm_width = st->ofm.width * stride_x + > - st->ifm.width - (st->ifm.pad_left + st->ifm.pad_right); > + st->ifm.width * dilation_x - > + (st->ifm.pad_left + st->ifm.pad_right); > > if (ifm_height < 0 || ifm_width < 0) > return -EINVAL; [Severity: Critical] This isn't a regression introduced by this patch, but does the command stream validation in feat_matrix_size() fail to check the maximum accessed address within intermediate tiles? Looking at drivers/accel/ethosu/ethosu_gem.c in 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); The validation loop only checks the memory bounds by testing the origin of the first tile (xi = 0, yi = 0) and the extremities of the last tile (xi = 1, yi = 1), but skips checking the inner boundaries of intermediate tiles. If a user-space application constructs a command stream defining a feature map that spans multiple tiles (e.g., storage == 0), could it set base[0] near the end of the valid GEM buffer and set a large width0 for the first tile, while setting base[1] to the beginning of the buffer? Because the check for x = 0 (Tile 0) uses base[0] + 0, and the check for x = x_max (Tile 1) uses base[1] + offset, both could pass validation. However, the maximum offset within Tile 0 (base[0] + width0 * stride_x) is never verified by the logic. Could this allow the NPU to execute the command stream and access out-of-bounds memory up to this unchecked boundary? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=13
