Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues: - [Critical] The hardware's state registers and BASEP (base pointer) registers are not reset between jobs, allowing an attacker to omit configuration commands and hijack memory regions from previously executed jobs. - [Critical] The `dma_length()` function incorrectly calculates memory bounds for negative DMA strides, drastically underestimating the maximum address and failing to check for buffer underflows. - [Medium] The command-stream validator employs a default-allow policy for unknown commands, passing unrecognized NPU commands directly to the hardware without any safety checks. -- commit 3012502d7ce6f9c86b531a92e4564ed74d340366 Author: Rob Herring (Arm) <[email protected]> accel: ethosu: Reject unsupported commands This commit updates the command-stream validator to reject unsupported commands like branches, IRQs, and certain DMA configurations. It prevents commands that bypass validation state or access unchecked index buffers. > diff --git a/drivers/accel/ethosu/ethosu_gem.c > b/drivers/accel/ethosu/ethosu_gem.c > index 3d1f4121db4f4..2aafbfe95a8c2 100644 > --- a/drivers/accel/ethosu/ethosu_gem.c > +++ b/drivers/accel/ethosu/ethosu_gem.c [ ... ] > @@ -450,6 +450,9 @@ static int ethosu_gem_cmdstream_copy_and_validate(struct > drm_device *ddev, > } > > switch (cmd) { > + case NPU_OP_BRANCH: > + case NPU_OP_IRQ: > + return -EINVAL; [Severity: Medium] This is a pre-existing issue, but is the validator using a default-allow policy for unknown commands? If a user submits a command stream containing unrecognized opcodes (such as undocumented commands or those from a minor hardware revision), the validator hits the default case: drivers/accel/ethosu/ethosu_gem.c:ethosu_gem_cmdstream_copy_and_validate() { ... switch (cmd) { ... default: break; } ... } This ignores the command without calculating memory bounds for it, allowing it to be copied to the execution buffer. The hardware could then execute it, potentially accessing physical memory outside the validated GEM bounds. Should the validator fail closed by returning -EINVAL for unrecognized commands? [ ... ] > @@ -673,13 +684,19 @@ static int > ethosu_gem_cmdstream_copy_and_validate(struct drm_device *ddev, > break; > > case NPU_SET_DMA0_SRC_REGION: [Severity: Critical] This isn't a bug introduced by this patch, but what happens if an attacker intentionally omits these configuration commands? If a command stream contains NPU_OP_DMA_START but omits NPU_SET_DMA0_SRC_REGION and NPU_SET_DMA0_DST_REGION, the regions default to -1 because cmd_state_init() initializes the state with 0xff: drivers/accel/ethosu/ethosu_gem.c:cmd_state_init() { memset(st, 0xff, sizeof(*st)); } In dma_length(), the upper bound update is bypassed because of the region check: drivers/accel/ethosu/ethosu_gem.c:dma_length() { if (dma->region >= 0) { ... info->region_size[dma->region] = max(...); } } Because region_size remains 0, the job can be submitted without a valid region_bo_handle. When ethosu_job_hw_submit() runs, it only programs NPU_REG_BASEP for explicitly provided regions: drivers/accel/ethosu/ethosu_job.c:ethosu_job_hw_submit() { ... writel_relaxed(lower_32_bits(bo->dma_addr), dev->regs + NPU_REG_BASEP(region)); ... } This leaves other BASEP registers retaining physical addresses from the previously executed job. The NPU hardware retains internal state across jobs, so it could use the un-overridden regions from the previous job, leading to cross-process memory leaks or physical memory corruption. [Severity: Critical] This is also a pre-existing issue, but how are negative DMA strides handled during length calculation? drivers/accel/ethosu/ethosu_gem.c:dma_length() { ... len += dma->stride[0]; if (check_mul_overflow(len, (u64)dma_st->size0, &len)) return U64_MAX; ... } If a negative stride is provided (e.g., stride[0] = -len and offset = 0), the logic incorrectly subtracts it before multiplication. This results in len = 0, making the maximum address calculation 0 and bypassing the upper bound check. Since the hardware DMA engine iterates backwards, it would access physical memory below BASEP. The validator never calculates the minimum address accessed nor checks for this lower bound underflow, which can allow out-of-bounds physical memory access. > + if (param & NPU_DMA_REGION_INDEX_MODE) > + return -EINVAL; > if (param & 0x100) > st.dma.src.region = -1; > else > st.dma.src.region = param & 0x7; > st.dma.mode = (param >> 9) & 0x3; > + if (st.dma.mode == 3) > + return -EINVAL; > break; -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=9
