Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues: - [Critical] Incomplete U65 storage encoding override causes inconsistent tile address calculation and broken validation. Pre-existing issues: - [Critical] Insufficient bounds checking of intermediate feature map tiles due to evaluating start coordinates instead of maximum accessed coordinates. -- commit 6ed551b31329946a632c14e3d13b0adf33231586 Author: Rob Herring (Arm) <[email protected]> accel: ethosu: Validate all feature map tiles This commit updates the command-stream validator to check the final coordinate of every tile touched by an operation instead of just the final feature-map coordinate. It also treats U65 feature maps as 2x2 tiled. > diff --git a/drivers/accel/ethosu/ethosu_gem.c > b/drivers/accel/ethosu/ethosu_gem.c > index 2aafbfe95a8c2..a042e650f626a 100644 > --- a/drivers/accel/ethosu/ethosu_gem.c > +++ b/drivers/accel/ethosu/ethosu_gem.c [ ... ] > @@ -259,6 +259,72 @@ static u64 feat_matrix_length(struct ethosu_device *edev, > return addr; > } > > +static int feat_matrix_check_location(struct ethosu_device *edev, [ ... ] > +static int feat_matrix_size(struct ethosu_device *edev, > + struct ethosu_validated_cmdstream_info *info, > + struct feat_matrix *fm, > + u32 x, u32 y, u32 c, bool ofm, u64 *max_len) > +{ > + u32 storage = ethosu_is_u65(edev) ? 0 : fm->precision >> 14; [Severity: Critical] Does this U65 storage encoding override create an inconsistency with how feat_matrix_length() calculates the storage? In feat_matrix_size(), storage is forced to 0 for U65. However, looking at feat_matrix_length(), it still decodes storage using the precision bits: static u64 feat_matrix_length(...) { u32 element_size, storage = fm->precision >> 14; ... } Could a manipulated command stream use this discrepancy to pass validation on one tile (using storage = 0) while the hardware accesses a different, unvalidated tile base address (using storage = 1)? > + 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, fm, > + xi ? x : 0, > + yi ? y : 0, c, > ofm, > + max_len); [Severity: Critical] This is a pre-existing issue, but does this loop evaluate the absolute coordinate 0 instead of the maximum valid coordinate for the intermediate tile? By checking (0, y) and (x, y), it appears we skip the maximum local coordinate accessed in the intermediate tile (such as fm->width0). Could this allow a command stream with an inflated stride to pass validation here but cause the hardware to perform an out-of-bounds DMA access? > + if (ret) > + return ret; > + } > + } > + return 0; > + } > + > + if (storage == 1) { > + ret = feat_matrix_check_location(edev, info, fm, x, 0, c, > + ofm, max_len); [Severity: Critical] This is a pre-existing issue, but similar to the 2x2 case above, is tile 0 checked at y=0 instead of its maximum accessed extent (fm->height[0])? This might leave its size bounds not fully validated. > + if (ret) > + return ret; [ ... ] -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=10
