Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [Critical] The command stream validation silently ignores unsupported/unknown formats (values 2 and 3), under-calculating the required buffer size and allowing out-of-bounds memory accesses. - [Critical] Buffer bounds validation incorrectly evaluates the memory footprint only at the maximum dimension indices, missing active tiles and failing on negative strides or overflows. -- --- Patch [1]: [PATCH 1/2] accel: ethosu: Fix element size accounting for cmd stream validation --- commit 295d16016ba05ed08c0ba3586bc6d83af9ebb17a Author: Rob Herring (Arm) <[email protected]> accel: ethosu: Fix element size accounting for cmd stream validation There are 2 issues with the element size handling in the command stream validation which result in too small of a size calculated when the element size is 16/32/64 bits. For NHWC format, the element size is simply missing from the calculation. > diff --git a/drivers/accel/ethosu/ethosu_gem.c > b/drivers/accel/ethosu/ethosu_gem.c > index 3401883e207fc..1daff32610c71 100644 > --- a/drivers/accel/ethosu/ethosu_gem.c > +++ b/drivers/accel/ethosu/ethosu_gem.c > @@ -194,7 +194,7 @@ static u64 dma_length(struct > ethosu_validated_cmdstream_info *info, [Severity: Critical] This is a pre-existing issue, but does the buffer bounds validation in feat_matrix_length() correctly evaluate the memory footprint for all tiles and negative strides? Looking at the calculation earlier in the function: switch (storage) { case 0: if (x >= fm->width0 + 1) { x -= fm->width0 + 1; tile += 1; } if (y >= fm->height[tile] + 1) { y -= fm->height[tile] + 1; tile += 2; } break; case 1: if (y >= fm->height[1] + 1) { y -= fm->height[1] + 1; tile = 2; } else if (y >= fm->height[0] + 1) { y -= fm->height[0] + 1; tile = 1; } break; By evaluating the maximum memory requirement only at the maximum y dimension, the driver seems to fail validating accesses at y=0, which would result in a higher address if stride_y is negative. Additionally, when a tensor spans multiple tiles (e.g. tile = 2), the validation logic checks only the last tile, entirely skipping bounds checks for the earlier tiles. Could a userspace process bypass validation by setting these unchecked bases to out-of-bounds addresses, leading to out-of-bounds DMA? > static u64 feat_matrix_length(struct ethosu_validated_cmdstream_info *info, > struct feat_matrix *fm, > - u32 x, u32 y, u32 c) > + u32 x, u32 y, u32 c, bool ofm) > { > u32 element_size, storage = fm->precision >> 14; > int tile = 0; [ ... ] > @@ -231,10 +231,11 @@ static u64 feat_matrix_length(struct > ethosu_validated_cmdstream_info *info, > > switch ((fm->precision >> 6) & 0x3) { // format [Severity: Critical] This isn't a bug introduced by this patch, but what happens here if a userspace process submits a command stream with a precision format of 2 or 3? Since the switch lacks handling for cases 2 and 3, it seems to silently fall through and bypass adding the x and c components to the address offset. Does this cause the validation routine to compute a falsely small memory footprint, potentially allowing the command to execute on a smaller-than-required buffer and perform out-of-bounds DMA access? > case 0: //nhwc: > - addr += x * fm->stride_x + c; > + element_size = BIT((fm->precision >> (ofm ? 1 : 2)) & 0x3); > + addr += x * fm->stride_x + c * element_size; > break; > case 1: //nhcwb16: > - element_size = BIT((fm->precision >> 1) & 0x3); > + element_size = BIT((fm->precision >> (ofm ? 1 : 2)) & 0x3); > > addr += (c / 16) * fm->stride_c + (16 * x + (c & 0xf)) * > element_size; > break; [ ... ] -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
