[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@xxxxxxxxx>
---
 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