From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: accel/ethosu: reject DMA commands with uninitialized length Date: Mon, 25 May 2026 17:05:58 +1000 Message-ID: In-Reply-To: <20260524130319.12747-1-meatuni001@gmail.com> References: <20260524130319.12747-1-meatuni001@gmail.com> <20260524130319.12747-1-meatuni001@gmail.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review **The fix itself is correct and well-placed:** ```c + if (len =3D=3D U64_MAX) + return U64_MAX; ``` Adding this check at line 166 (before the stride arithmetic at lines 167-17= 2) prevents `U64_MAX` from wrapping through addition and multiplication, wh= ich is the right approach. It's consistent with the sentinel pattern used e= lsewhere in the driver (e.g., `feat_matrix_length()` checks `fm->base[tile]= =3D=3D U64_MAX` at line 213). **Commit message inaccuracies:** 1. The message says *"The caller's U64_MAX check then passes"* =E2=80=94 bu= t looking at the `NPU_OP_DMA_START` handler (lines 395-403), there is **no*= * U64_MAX check on the return value of `dma_length()`. The values `srclen`/= `dstlen` are computed and logged but never validated. The only downstream p= rotection is the `region_size` bounds check in `ethosu_job.c:453`, which co= mpares against the GEM buffer size. 2. The message says *"region_size[] stays 0"* =E2=80=94 this is actually co= rrect only when `dma->region` is `-1` (the `0xff` sentinel for `s8`), becau= se the `dma->region >=3D 0` guard at line 174 prevents the `region_size` up= date. But in that case the DMA wouldn't reference any region at all. If use= rspace sets a valid `dma->region` but omits `NPU_SET_DMA0_LEN`, the wrapped= small value *would* be written to `region_size`, which is the more interes= ting attack vector =E2=80=94 and this patch does correctly fix that case. **Incomplete fix =E2=80=94 no validation at the DMA caller:** The `NPU_OP_DMA_START` handler at line 395 does not check whether `dma_leng= th()` returned `U64_MAX`. Compare with how `feat_matrix_length()` return va= lues are checked in `calc_sizes()`. After this patch, `dma_length()` correc= tly returns `U64_MAX` for uninitialized lengths, but nothing in the DMA pat= h actually acts on that sentinel. The `region_size` write is skipped (due t= o `region >=3D 0` guard), and execution continues normally. A more complete= fix would add: ```c case NPU_OP_DMA_START: srclen =3D dma_length(info, &st.dma, &st.dma.src); dstlen =3D dma_length(info, &st.dma, &st.dma.dst); + if (srclen =3D=3D U64_MAX || dstlen =3D=3D U64_MAX) + return -EINVAL; ``` **Minor style note:** The patch applies cleanly against the diff context sh= own, and the 3-line addition is minimal and non-invasive. The `Fixes:` tag = and `Cc: stable` are appropriate if the maintainers agree this is a securit= y-relevant fix. **Summary:** The arithmetic hardening in `dma_length()` is correct and wort= h having, but the commit message's exploitation narrative doesn't fully mat= ch the code. The author should also consider adding explicit U64_MAX valida= tion in the `NPU_OP_DMA_START` handler to make the defense complete, simila= r to how other operation handlers validate their length computations. --- Generated by Claude Code Patch Reviewer