Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues: - [High] Userspace can bypass GEM bounds validation for secondary weight and scale buffers by explicitly setting their length to U32_MAX or omitting their base address initialization. - [High] The feature matrix bounds calculation in `feat_matrix_length()` only evaluates the base address of the final active tile, ignoring earlier tiles and allowing OOB DMA. - [High] DMA length validation fails to check the lower bound for operations with negative strides, enabling OOB accesses before the GEM object's base IOVA. -- commit 7486f97fafb727f5b9b006f0446971da1e03b741 Author: Muhammad Bilal <[email protected]> Subject: accel/ethosu: include secondary weight/scale extents in region_size[] accounting This commit updates calc_sizes() to account for the extents of secondary weight and scale buffers when calculating required GEM buffer sizes. Previously, these secondary buffers were omitted from the region size calculations, which could underestimate the required size. > diff --git a/drivers/accel/ethosu/ethosu_gem.c > b/drivers/accel/ethosu/ethosu_gem.c > --- a/drivers/accel/ethosu/ethosu_gem.c > +++ b/drivers/accel/ethosu/ethosu_gem.c > @@ -285,6 +285,8 @@ static int calc_sizes(struct drm_device *ddev, > } > > if (weight) { > + int i; > + > dev_dbg(ddev->dev, "op %d: W:%d:0x%llx-0x%llx\n", > op, st->weight[0].region, st->weight[0].base, > st->weight[0].base + st->weight[0].length - 1); > @@ -294,6 +296,14 @@ static int calc_sizes(struct drm_device *ddev, > info->region_size[st->weight[0].region] = > max(info->region_size[st->weight[0].region], > st->weight[0].base + st->weight[0].length); > + for (i = 1; i < ARRAY_SIZE(st->weight); i++) { > + if (st->weight[i].base == U64_MAX || > + st->weight[i].length == U32_MAX) > + continue; [Severity: High] This isn't a bug introduced by this patch, but does this sentinel check in calc_sizes() allow userspace to bypass validation for secondary weight buffers? If malicious userspace explicitly sends a command with a length of U32_MAX and an out-of-bounds base address, this continue statement skips updating the region size. Could the NPU hardware still execute the command using the out-of-bounds base address, resulting in an out-of-bounds DMA access? Alternatively, if the base address is omitted and left as U64_MAX, could the hardware use a stale, out-of-bounds register offset from a previous operation? > + info->region_size[st->weight[0].region] = > + max(info->region_size[st->weight[0].region], > + st->weight[i].base + st->weight[i].length); > + } > } > > if (scale) { [ ... ] > @@ -306,6 +316,11 @@ static int calc_sizes(struct drm_device *ddev, > info->region_size[st->scale[0].region] = > max(info->region_size[st->scale[0].region], > st->scale[0].base + st->scale[0].length); > + if (st->scale[1].base != U64_MAX && > + st->scale[1].length != U32_MAX) > + info->region_size[st->scale[0].region] = [Severity: High] This isn't a bug introduced by this patch, but the same sentinel bypass issue exists here for the secondary scale buffer. Does this permit arbitrary out-of-bounds DMA? > + max(info->region_size[st->scale[0].region], > + st->scale[1].base + st->scale[1].length); > } > > len = feat_matrix_length(info, &st->ofm, st->ofm.width, > st->ofm.height[2], st->ofm.depth); [Severity: High] This is a pre-existing issue, but does feat_matrix_length() properly validate the base addresses of all tiles? When calculating the feature matrix bounds in feat_matrix_length() in drivers/accel/ethosu/ethosu_gem.c, the base address is validated like this: if (fm->base[tile] == U64_MAX) return U64_MAX; addr = fm->base[tile] + y * fm->stride_y; Could userspace set the base address of the first tile to an arbitrary out-of-bounds address, and place the maximum spatial coordinates such that they evaluate to the final tile? If so, the validation would only check the base address of the final tile and ignore the earlier tiles, while the NPU processes all tiles and performs an out-of-bounds DMA at the malicious address. [Severity: High] This is another pre-existing issue, but does the bounds check logic in dma_length() in drivers/accel/ethosu/ethosu_gem.c correctly handle negative strides? The function updates the upper bound without checking the lower bound against 0: if (dma->stride[0] < 0 && (u64)(-dma->stride[0]) > len) return U64_MAX; len += dma->stride[0]; Since the NPU supports negative strides that move the access pointer backwards, a negative stride with a small offset could evaluate to a negative address and wrap around. Could this allow the NPU to perform DMA accesses to memory locations before the base address of the GEM object, bypassing memory protections? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
