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.

  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(-)

-- 
2.51.0

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(-)
>