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

[Eva Crystal <0xiviel@gmail.com>]

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