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

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

[Eva Crystal]

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

[PATCH 1/4] accel/amdxdna: validate the command payload regardless of the size argument

[Eva Crystal]

amdxdna_cmd_get_payload() performs its bounds check - that the command
header's count field does not describe a payload larger than the command
BO - only when the caller asks for a size:

	if (size) {
		count = FIELD_GET(AMDXDNA_CMD_COUNT, cmd->header);
		if (unlikely(count <= num_masks || ...  > abo->mem.size)) {
			*size = 0;
			return NULL;
		}
		*size = (count - num_masks) * sizeof(u32);
	}
	return &cmd->data[num_masks];

A caller passing NULL therefore receives a pointer into the command BO
that has never been checked against the BO's size, and no way to learn
that the header was malformed. The count field is written by user space:
the command BO is mapped into the submitting process and can be rewritten
after submission.

The one such caller today is amdxdna_cmd_set_error(), which reads
cc->command_count, writes cc->error_index and reads cc->data[0] - offsets
4, 12 and 28 into the payload. That is safe as things stand, because a
command BO is created through drm_gem_shmem_create() and its size is
always PAGE_ALIGN()ed, so any BO that can be vmap()ed is at least
PAGE_SIZE; a zero-sized BO fails vmap() and is rejected by the !cmd test
one line earlier. This is not a fix for a reachable bug.

It is, however, a validation step that a caller can silently opt out of,
guarding a structure whose contents user space controls, and the safety
of the only NULL caller rests on a page-alignment invariant established
three call levels away. Make the check unconditional and report the
failure to every caller, so that the guarantee does not depend on which
arguments the caller happened to pass.

amdxdna_cmd_set_error() is updated to handle the NULL it can now receive.

Signed-off-by: Eva Crystal <0xiviel@gmail.com>
---
 drivers/accel/amdxdna/amdxdna_ctx.c | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/drivers/accel/amdxdna/amdxdna_ctx.c b/drivers/accel/amdxdna/amdxdna_ctx.c
index 5315466..163b5fc 100644
--- a/drivers/accel/amdxdna/amdxdna_ctx.c
+++ b/drivers/accel/amdxdna/amdxdna_ctx.c
@@ -106,17 +106,19 @@ void *amdxdna_cmd_get_payload(struct amdxdna_gem_obj *abo, u32 *size)
 	else
 		num_masks = 1 + FIELD_GET(AMDXDNA_CMD_EXTRA_CU_MASK, cmd->header);
 
-	if (size) {
-		count = FIELD_GET(AMDXDNA_CMD_COUNT, cmd->header);
-		if (unlikely(count <= num_masks ||
-			     count * sizeof(u32) +
-			     offsetof(struct amdxdna_cmd, data[0]) >
-			     abo->mem.size)) {
+	count = FIELD_GET(AMDXDNA_CMD_COUNT, cmd->header);
+	if (unlikely(count <= num_masks ||
+		     count * sizeof(u32) +
+		     offsetof(struct amdxdna_cmd, data[0]) >
+		     abo->mem.size)) {
+		if (size)
 			*size = 0;
-			return NULL;
-		}
-		*size = (count - num_masks) * sizeof(u32);
+		return NULL;
 	}
+
+	if (size)
+		*size = (count - num_masks) * sizeof(u32);
+
 	return &cmd->data[num_masks];
 }
 
@@ -159,6 +161,9 @@ int amdxdna_cmd_set_error(struct amdxdna_gem_obj *abo,
 
 	if (amdxdna_cmd_get_op(abo) == ERT_CMD_CHAIN) {
 		cc = amdxdna_cmd_get_payload(abo, NULL);
+		if (!cc)
+			return -EINVAL;
+
 		cc->error_index = (cmd_idx < cc->command_count) ? cmd_idx : 0;
 		abo = amdxdna_gem_get_obj(client, cc->data[0], AMDXDNA_BO_SHARE);
 		if (!abo)
-- 
2.53.0

[PATCH 2/4] accel/amdxdna: bound the command error payload length

[Eva Crystal]

amdxdna_cmd_set_error() computes the length of the region it scribbles
over from the BO size, without a floor:

	memset(cmd->data, 0xff, abo->mem.size - sizeof(*cmd));
	if (err_data)
		memcpy(cmd->data, err_data, min(size, abo->mem.size - sizeof(*cmd)));

abo->mem.size is a size_t and sizeof(struct amdxdna_cmd) is 4 - the
struct is a u32 header followed by a flexible array. A BO smaller than
four bytes therefore turns both lengths into a value near SIZE_MAX, and
the min() in the memcpy offers no protection because the underflowed
value is the larger operand.

No such BO can reach this function today. Command BOs are created by
drm_gem_shmem_create(), which PAGE_ALIGN()s the size, so mem.size is
either 0 or at least PAGE_SIZE. Zero is reachable - PAGE_ALIGN() wraps
for sizes above ULLONG_MAX - PAGE_SIZE + 1, and nothing rejects it on
the share-BO path - but a zero-sized BO cannot be vmap()ed, because
vmap() refuses a zero-page mapping, so amdxdna_gem_vmap() returns NULL
and the !cmd test above rejects the BO before the subtraction. This is
not a fix for a reachable bug.

That leaves an unguarded size_t subtraction feeding a memset() length,
whose safety depends on a property of a different allocator and on
vmap()'s behaviour for a zero-page request. Compute the length once,
reject a BO too small to hold the header, and use the result for both
the memset() and the memcpy() bound.

Signed-off-by: Eva Crystal <0xiviel@gmail.com>
---
 drivers/accel/amdxdna/amdxdna_ctx.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/accel/amdxdna/amdxdna_ctx.c b/drivers/accel/amdxdna/amdxdna_ctx.c
index 163b5fc..c24bf1c 100644
--- a/drivers/accel/amdxdna/amdxdna_ctx.c
+++ b/drivers/accel/amdxdna/amdxdna_ctx.c
@@ -152,6 +152,7 @@ int amdxdna_cmd_set_error(struct amdxdna_gem_obj *abo,
 	struct amdxdna_client *client = job->hwctx->client;
 	struct amdxdna_cmd *cmd = amdxdna_gem_vmap(abo);
 	struct amdxdna_cmd_chain *cc = NULL;
+	size_t data_size;
 
 	if (!cmd)
 		return -ENOMEM;
@@ -173,9 +174,16 @@ int amdxdna_cmd_set_error(struct amdxdna_gem_obj *abo,
 			return -ENOMEM;
 	}
 
-	memset(cmd->data, 0xff, abo->mem.size - sizeof(*cmd));
+	if (abo->mem.size < sizeof(*cmd)) {
+		if (cc)
+			amdxdna_gem_put_obj(abo);
+		return -EINVAL;
+	}
+	data_size = abo->mem.size - sizeof(*cmd);
+
+	memset(cmd->data, 0xff, data_size);
 	if (err_data)
-		memcpy(cmd->data, err_data, min(size, abo->mem.size - sizeof(*cmd)));
+		memcpy(cmd->data, err_data, min(size, data_size));
 
 	if (cc)
 		amdxdna_gem_put_obj(abo);
-- 
2.53.0

[PATCH 3/4] accel/amdxdna: release the chained command BO when vmap fails

[Eva Crystal]

When amdxdna_cmd_set_error() follows a command chain it takes a
reference on the BO named by the chain:

	abo = amdxdna_gem_get_obj(client, cc->data[0], AMDXDNA_BO_SHARE);
	if (!abo)
		return -EINVAL;
	cmd = amdxdna_gem_vmap(abo);
	if (!cmd)
		return -ENOMEM;

and drops it at the end of the function under "if (cc)". The -ENOMEM
path returns before reaching that, so the reference taken by
amdxdna_gem_get_obj() is leaked and the GEM object is never freed.

amdxdna_gem_vmap() fails only if drm_gem_vmap() fails, which needs
memory pressure or an exporter that refuses the mapping, so this is a
small leak on a rare path rather than something a caller can drive at
will. It is still a leak, and the chain BO handle comes from a command
buffer user space can write.

Drop the reference before returning.

Signed-off-by: Eva Crystal <0xiviel@gmail.com>
---
 drivers/accel/amdxdna/amdxdna_ctx.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/accel/amdxdna/amdxdna_ctx.c b/drivers/accel/amdxdna/amdxdna_ctx.c
index c24bf1c..7a61e83 100644
--- a/drivers/accel/amdxdna/amdxdna_ctx.c
+++ b/drivers/accel/amdxdna/amdxdna_ctx.c
@@ -170,8 +170,10 @@ int amdxdna_cmd_set_error(struct amdxdna_gem_obj *abo,
 		if (!abo)
 			return -EINVAL;
 		cmd = amdxdna_gem_vmap(abo);
-		if (!cmd)
+		if (!cmd) {
+			amdxdna_gem_put_obj(abo);
 			return -ENOMEM;
+		}
 	}
 
 	if (abo->mem.size < sizeof(*cmd)) {
-- 
2.53.0

[PATCH 4/4] accel/amdxdna: check the command payload before using it in the exec requests

[Eva Crystal]

aie2_init_exec_dpu_req() takes the payload of a command BO and subtracts
the fixed header from its length before establishing that the length
covers the header at all:

	sn = amdxdna_cmd_get_payload(cmd_bo, &cmd_len);
	if (cmd_len - sizeof(*sn) > sizeof(dpu_req->payload))
		return -EINVAL;
	...
	dpu_req->inst_buf_addr = sn->buffer;
	memcpy(dpu_req->payload, sn->prop_args, cmd_len - sizeof(*sn));

It also dereferences sn without testing it, although
amdxdna_cmd_get_payload() returns NULL for a command header whose count
field does not fit the BO, setting cmd_len to 0 as it does so.

Both work out today, and for the same reason: cmd_len is a u32 and
sizeof() is a size_t, so the subtraction is evaluated in 64-bit. A short
command produces a value near 2^64, which is larger than the payload
field, so the test returns -EINVAL before sn is dereferenced and before
the memcpy. The NULL case is caught by the same comparison, because
cmd_len is then 0.

The guarantee is therefore supplied entirely by the operand types. A
later change that computes the difference into a u32 first - as the
sibling slot-filling functions in this file already do - would truncate
the underflow to a small value, and the function would dereference NULL
and copy from a short payload. Those siblings, aie2_cmdlist_fill_dpu()
and friends, all carry an explicit "cmd_len < sizeof(*sn)" test for
exactly this reason; this path was left without one.

Add the explicit length and NULL tests here, matching the siblings, and
add the missing NULL test to aie2_init_exec_cu_req(), which copies
cmd_len bytes from a pointer it likewise never checks. No behavioural
change is intended: every input rejected by the new tests is already
rejected today.

Signed-off-by: Eva Crystal <0xiviel@gmail.com>
---
 drivers/accel/amdxdna/aie2_message.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/accel/amdxdna/aie2_message.c b/drivers/accel/amdxdna/aie2_message.c
index 3028968..ab9e7c4 100644
--- a/drivers/accel/amdxdna/aie2_message.c
+++ b/drivers/accel/amdxdna/aie2_message.c
@@ -556,7 +556,7 @@ static int aie2_init_exec_cu_req(struct amdxdna_gem_obj *cmd_bo, void *req,
 	void *cmd;
 
 	cmd = amdxdna_cmd_get_payload(cmd_bo, &cmd_len);
-	if (cmd_len > sizeof(cu_req->payload))
+	if (!cmd || cmd_len > sizeof(cu_req->payload))
 		return -EINVAL;
 
 	cu_req->cu_idx = amdxdna_cmd_get_cu_idx(cmd_bo);
@@ -578,7 +578,8 @@ static int aie2_init_exec_dpu_req(struct amdxdna_gem_obj *cmd_bo, void *req,
 	u32 cmd_len;
 
 	sn = amdxdna_cmd_get_payload(cmd_bo, &cmd_len);
-	if (cmd_len - sizeof(*sn) > sizeof(dpu_req->payload))
+	if (!sn || cmd_len < sizeof(*sn) ||
+	    cmd_len - sizeof(*sn) > sizeof(dpu_req->payload))
 		return -EINVAL;
 
 	dpu_req->cu_idx = amdxdna_cmd_get_cu_idx(cmd_bo);
-- 
2.53.0