Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Critical] For Ethos-U65, the shared DMA skip values are parsed and assigned only to the destination stride, leaving the source stride uninitialized (zero). This causes the driver to severely underestimate the required source buffer size, allowing userspace to bypass bounds validation and trigger out-of-bounds memory accesses. --
commit f80309886975bd6af6358046bb0e792268687173 Author: Rob Herring (Arm) <[email protected]> accel: ethosu: Split U65 and U85 DMA length validation This commit splits the DMA length calculation in the Ethos-U driver into separate versions for U65 and U85 to handle their slightly different programming models. The U65 uses skip values added to the size, while the U85 uses independent signed stride values. > diff --git a/drivers/accel/ethosu/ethosu_gem.c > b/drivers/accel/ethosu/ethosu_gem.c > index c3dad3f80c34b..559fbf55f12d5 100644 > --- a/drivers/accel/ethosu/ethosu_gem.c > +++ b/drivers/accel/ethosu/ethosu_gem.c [ ... ] > @@ -225,40 +225,92 @@ static bool dma_params_valid(struct ethosu_device > *edev, struct cmd_state *st, [ ... ] > +static u64 dma_length_u65(struct ethosu_validated_cmdstream_info *info, > + struct dma_state *dma_st, > + struct dma *dma) > { > s8 mode = dma->mode; > u64 len = dma->len; > > - if (!dma_params_valid(edev, st, dma_st, dma, region_cmd, addr_cmd)) > - return U64_MAX; > + if (mode >= 1) { > + if (check_add_overflow(len, (u64)dma->stride[0], &len) || > + check_mul_overflow(len, (u64)dma_st->size0, &len)) > + return U64_MAX; > + } [Severity: Critical] Does this code underestimate the bounding memory range for the source buffer? When evaluating the source DMA length for U65, dma->stride[0] will be 0 because ethosu_gem_cmdstream_copy_and_validate() only assigns the skip value to the destination stride. This bypasses the skip value expansion and could allow userspace to bypass bounds validation, potentially triggering out-of-bounds memory accesses. [ ... ] > @@ -1047,16 +1099,28 @@ static int > ethosu_gem_cmdstream_copy_and_validate(struct drm_device *ddev, > st.dma.size1 = param; > break; > case NPU_SET_DMA0_SRC_STRIDE0: > - st.dma.src.stride[0] = ((s64)addr << 24) >> 24; > + if (ethosu_is_u65(edev)) > + st.dma.dst.stride[0] = addr; [Severity: Critical] Is it intentional that the user-provided shared stride is only assigned to the destination stride here? This leaves the source stride, st.dma.src.stride[0], uninitialized (0) for U65, leading to the regression in dma_length_u65() where length bounds are underestimated. Should this also be assigned to st.dma.src.stride[0] for U65? > + else > + st.dma.src.stride[0] = sign_extend64(addr, 39); > break; > case NPU_SET_DMA0_SRC_STRIDE1: > - st.dma.src.stride[1] = ((s64)addr << 24) >> 24; > + if (ethosu_is_u65(edev)) > + st.dma.dst.stride[1] = addr; [Severity: Critical] Does this same regression occur for stride 1? It appears this also leaves st.dma.src.stride[1] uninitialized for U65. > + else > + st.dma.src.stride[1] = sign_extend64(addr, 39); > break; -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=17
