Re: [PATCH 0/4] accel/amdxdna: harden command BO payload validation

[Lizhi Hou <lizhi.hou@amd.com>]

On 9/12/26 01:10, Eva Crystal wrote:
> These came out of a read of the command submission path in
> drivers/accel/amdxdna.
>
> Where to spend review attention: patch 3 is a real fix - a leaked GEM
> reference on an error path. Patches 1, 2 and 4 are hardening. I could
> not reach any of those three, and each commit message says so in as many
> words and explains what currently prevents it. I would rather be plain
> about that up front than have you read three messages looking for a bug
> that is not there.
>
> What the three have in common is that a check on user-controlled data is
> either skipped, or holds only because of a property established
> somewhere else - an allocator that page-aligns, vmap() refusing a
> zero-page mapping, or the integer promotion rules. Those properties hold
> today. They are not local to the code that depends on them, and two of
> the three sit next to siblings that already carry the explicit check.

I will run some tests against these three.

Thanks,

Lizhi

>
>    Patch 1 makes amdxdna_cmd_get_payload()'s bounds check unconditional.
>            It is currently inside "if (size)", so a caller passing NULL
>            gets an unvalidated pointer into the command BO. The single
>            NULL caller is safe because command BOs are always
>            PAGE_ALIGN()ed.  [hardening]
>
>    Patch 2 gives the error-path memset()/memcpy() in
>            amdxdna_cmd_set_error() a floor. The length is
>            "abo->mem.size - sizeof(*cmd)" with no check that mem.size is
>            at least 4. A zero-sized BO is creatable, but cannot be
>            vmap()ed, so it is rejected a few lines earlier.  [hardening]
>
>    Patch 3 is an actual bug fix: the -ENOMEM path in
>            amdxdna_cmd_set_error() returns without dropping the reference
>            amdxdna_gem_get_obj() took on the chained command BO. Small
>            leak on a rare path, but a leak.  [fix]
>
>    Patch 4 adds the explicit short-length and NULL tests to
>            aie2_init_exec_dpu_req() and aie2_init_exec_cu_req(). The
>            length test is currently performed by subtracting a size_t
>            from a u32 and relying on the result being evaluated in
>            64-bit, so that a short command underflows to a value larger
>            than the destination. The slot-filling siblings in the same
>            file (aie2_cmdlist_fill_dpu() and friends) already have the
>            explicit "cmd_len < sizeof(*sn)" test; these two do not.
>            [hardening]
>
> No behavioural change is intended anywhere except patch 3. Every input
> the new tests reject is already rejected today.
>
> Based on v7.1.5. Compile-tested as an out-of-tree build against 7.1.5
> headers, no new warnings.
>
> Not runtime-tested, and I want to be explicit about that rather than
> leave it implied. I have the hardware - a Strix Point NPU, 1022:17f0,
> running npu_7.sbin 1.1.2.64 - and I am happy to run whatever you would
> like on it and report back. I did not want to send results I had not
> actually produced.
>
> I have deliberately not added Fixes: tags. I worked from release
> tarballs rather than a git tree and could not verify the introducing
> commits; someone with the history should add them if these are taken.
>
> Eva Crystal (4):
>    accel/amdxdna: validate the command payload regardless of the size argument
>    accel/amdxdna: bound the command error payload length
>    accel/amdxdna: release the chained command BO when vmap fails
>    accel/amdxdna: check the command payload before using it in the exec requests
>
>   drivers/accel/amdxdna/aie2_message.c |  5 +++--
>   drivers/accel/amdxdna/amdxdna_ctx.c  | 39 ++++++++++++++++++++++----------
>   2 files changed, 30 insertions(+), 14 deletions(-)
>