Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues: - [Critical] Incomplete bounds validation for multi-tile feature maps allows NPU out-of-bounds memory access (stride overflow). -- commit ed82a852cb38061cea72ccfbb8b9844fe471cc0f Author: Rob Herring (Arm) <[email protected]> accel: ethosu: Validate all feature map tiles The commit modifies the command-stream validator to check the final coordinate of every tile touched by an operation, rather than just the final feature-map coordinate. It also updates the treatment of U65 feature maps to be checked as 2x2 tiled. > diff --git a/drivers/accel/ethosu/ethosu_gem.c > b/drivers/accel/ethosu/ethosu_gem.c > index c913c95e48ae0..c54496fa08f4c 100644 > --- a/drivers/accel/ethosu/ethosu_gem.c > +++ b/drivers/accel/ethosu/ethosu_gem.c [ ... ] > @@ -423,6 +423,74 @@ static u64 feat_matrix_length(struct ethosu_device *edev, [ ... ] > +static int feat_matrix_size(struct ethosu_device *edev, > + struct ethosu_validated_cmdstream_info *info, > + struct cmd_state *st, struct feat_matrix *fm, > + enum feat_matrix_type type, > + u32 x, u32 y, u32 c, bool ofm, u64 *max_len) > +{ > + u32 storage = ethosu_is_u65(edev) ? 0 : fm->precision >> 14; > + int ret; > + > + *max_len = 0; > + > + 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 isn't a bug introduced by this patch, but it appears the validation logic might still be incomplete for intermediate tiles. In feat_matrix_size(), when checking storage == 0, the loop evaluates to 0 (the relative minimum coordinate) for intermediate tiles rather than checking the tile's maximum extent. Because feat_matrix_length() calculates offset by multiplying strides by the passed coordinates, passing 0 skips validating the maximum memory offset for tile 0 (which should be evaluated at fm->width0 and fm->height[0]). Could an ioctl command stream from unprivileged userspace, carrying a valid base address but artificially large strides, bypass bounds checking here and cause the NPU to access out-of-bounds memory? > + if (ret) > + return ret; > + } > + } > + return 0; > + } > + > + if (storage == 1) { > + ret = feat_matrix_check_location(edev, info, st, fm, type, x, > 0, c, > + ofm, max_len); > + if (ret) > + return ret; > + if (fm->height[0] < fm->height[1] && fm->height[1] <= y) { > + ret = feat_matrix_check_location(edev, info, st, fm, > type, x, > + fm->height[1], c, ofm, > + max_len); [Severity: Critical] This is also a pre-existing issue, but similar to the loop above, does this storage == 1 logic skip validating the maximum memory bound of tile 0? The code explicitly checks the boundaries at y = 0 and y = fm->height[1], but it seems to skip verifying the maximum extent of tile 0 at y = fm->height[0]. Would this allow unprivileged userspace to trigger out-of-bounds accesses during intermediate tile processing if the vertical stride is maliciously large? > + if (ret) > + return ret; > + } [ ... ] -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=10
