Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues: - [Medium] Missing READ_ONCE() when reading `cc->command_count` from a user-space mapped buffer introduces a Time-of-Check to Time-of-Use (TOCTOU) vulnerability. Pre-existing issues: - [High] Reference leak for the chained GEM object `abo` in `amdxdna_cmd_set_error()` when `amdxdna_gem_vmap()` fails. - [High] Unprotected access to user-mapped flexible array `cc->data[0]` causes data races and triggers UBSAN panics via `__counted_by`. -- commit a1b05572fd8c69d4959c5969b2a34b2e5ce07591 Author: Taimuraz Kaitmazov <[email protected]> Subject: accel/amdxdna: check the command chain payload before using it This commit introduces a new function amdxdna_cmd_get_chain() to ensure the chain payload of a BO is checked before use. It makes the size parameter mandatory in amdxdna_cmd_get_payload() and updates amdxdna_cmd_set_error() to utilize the new chain validation logic. > diff --git a/drivers/accel/amdxdna/amdxdna_ctx.c > b/drivers/accel/amdxdna/amdxdna_ctx.c > index 855da8c79a1cd..143cbbbf4b310 100644 > --- a/drivers/accel/amdxdna/amdxdna_ctx.c > +++ b/drivers/accel/amdxdna/amdxdna_ctx.c > [ ... ] > @@ -125,20 +125,42 @@ void *amdxdna_cmd_get_payload(struct amdxdna_gem_obj > *abo, u32 *size) > [ ... ] > +/* > + * Returns the chain payload of @abo, with @count set to a command count that > + * has been checked to fit. The chain fields live in a BO user space keeps > + * mapped, so nothing may read them without going through here. > + */ > +struct amdxdna_cmd_chain * > +amdxdna_cmd_get_chain(struct amdxdna_gem_obj *abo, u32 *count) > +{ > + struct amdxdna_cmd_chain *cc; > + u32 len, ccnt; > + > + cc = amdxdna_cmd_get_payload(abo, &len); > + if (!cc || len < sizeof(*cc)) > + return NULL; > + > + ccnt = cc->command_count; > + if (len < struct_size(cc, data, ccnt)) > + return NULL; > + > + *count = ccnt; [Severity: Medium] Since the memory for cc resides in a GEM object mapped to userspace, can the compiler perform multiple memory loads of cc->command_count here because it lacks a READ_ONCE() wrapper? If a user modifies cc->command_count concurrently between the struct_size() bounds check and the assignment to *count, could this bypass the newly added bounds check and pass a larger ccnt to the caller? > + return cc; > +} > [ ... ] > @@ -177,8 +199,13 @@ int amdxdna_cmd_set_error(struct amdxdna_gem_obj *abo, > cmd->header |= FIELD_PREP(AMDXDNA_CMD_STATE, error_state); > > if (amdxdna_cmd_get_op(abo) == ERT_CMD_CHAIN) { > - cc = amdxdna_cmd_get_payload(abo, NULL); > - cc->error_index = (cmd_idx < cc->command_count) ? cmd_idx : 0; > + u32 ccnt; > + > + cc = amdxdna_cmd_get_chain(abo, &ccnt); > + if (!cc || !ccnt) > + return -EINVAL; > + > + cc->error_index = (cmd_idx < ccnt) ? cmd_idx : 0; > abo = amdxdna_gem_get_obj(client, cc->data[0], > AMDXDNA_BO_SHARE); [Severity: High] This is a pre-existing issue, but since struct amdxdna_cmd_chain defines data as a flexible array u64 data[] __counted_by(command_count), does accessing cc->data[0] dynamically re-read cc->command_count from memory to assert 0 < command_count? If cc is backed by user-mapped memory, couldn't a malicious user concurrently write 0 to command_count, causing the dynamically validated access to fail and trigger a UBSAN kernel panic if CONFIG_UBSAN_BOUNDS is enabled? > if (!abo) > return -EINVAL; [Severity: High] This is a pre-existing issue, but does this function leak the reference to the chained GEM object abo if amdxdna_gem_vmap() fails? Looking at amdxdna_cmd_set_error(), it successfully looks up the chained BO using amdxdna_gem_get_obj(), which increments its reference count: 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; If amdxdna_gem_vmap() fails, the function returns -ENOMEM without calling amdxdna_gem_put_obj(abo). Can an unprivileged user trigger this by causing job failures under memory pressure or by crafting a BO that fails to map, leading to a memory leak and eventual Denial of Service? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
