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