Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [High] misc: arm-cla: max_regidx truncated to 0 on 32-bit architectures - [High] misc: arm-cla: partially overwritten context state left valid on error - [High] misc: arm-cla: NULL pointer dereference in cla_ctx_free_regs() - [High] misc: arm-cla: ctx->regs indexed by ctx_id instead of device index --
commit 3663e5cba865127a89c5cdad88c78f8b24a13ae0 Author: Jean-Philippe Brucker <[email protected]> misc/arm-cla: Accelerator context save and restore Add helpers to save and restore CLA context for later domain switching. The saved state includes DATA registers, LRESP, per-accelerator SRSTATE and any implementation-defined internal accelerator state exposed through the IAS register window. > diff --git a/drivers/misc/arm-cla/cla-regs.c b/drivers/misc/arm-cla/cla-regs.c > new file mode 100644 > index 0000000000000..2452a7601990f > --- /dev/null > +++ b/drivers/misc/arm-cla/cla-regs.c [ ... ] > +static int cla_regs_save_accel(struct cla_dev *dev, unsigned int accid, > + struct cla_regs_pl0 *regs, size_t *regstate_off) > +{ [ ... ] > + if (regstate_size) { > + ret = cla_op_regread(dev, accid, CLA_REG_IASn, regstate_size, > + regs->regstate + *regstate_off); [Severity: High] Does this code unconditionally fail on 32-bit architectures? cla_op_regread() calls cla_op_access_reg(). In that function: drivers/misc/arm-cla/cla-ops.c:cla_op_access_reg() { ... unsigned long max_regidx; ... case CLA_LAUNCH_OP_REGREAD: case CLA_LAUNCH_OP_REGWRITE: max_regidx = 0x100000000; break; ... if (WARN_ON(regidx + nregs > max_regidx)) return -EINVAL; } max_regidx is declared as an unsigned long and assigned the 33-bit constant 0x100000000. On 32-bit platforms, unsigned long is 32 bits wide, so the assignment silently truncates the value to 0. With max_regidx being 0, the bounds check regidx + nregs > 0 is unconditionally true since nregs must be greater than zero. This causes cla_op_access_reg() to unconditionally fail with -EINVAL, completely breaking the newly added context save and restore features here. > + if (ret) { > + cla_err(dev, "failed to save regstate: %d\n", ret); > + return ret; > + } > + } [ ... ] > +static int cla_regs_uninstall_pl0(struct cla_dev *dev, struct cla_ctx *ctx, > + unsigned int ctx_id) > +{ > + int i; > + int ret; > + unsigned int accid; > + size_t regstate_off = 0; > + struct cla_regs_pl0 *regs = ctx->regs[ctx_id]; [Severity: High] Does this code index the ctx->regs array with the wrong identifier? cla_ctx_alloc_regs() allocates ctx->regs as an array of size domain->nr_devs, where each element index corresponds directly to a specific device in the domain (domain->devs[i]). However, this uses ctx_id to index the array. ctx_id represents the logical context number within the domain, not the hardware device index. If a user allocates more contexts than the number of hardware devices (ctx_id >= domain->nr_devs), this causes an immediate out-of-bounds array access. Even if ctx_id is within bounds, using it instead of the device index results in reading and writing state to the wrong device's register array. > + > + /* > + * When we interrupt the user process in the middle of launching a > + * command, we have to wait for LRESP_PENDING to clear before we can > + * launch a new command or save the DATA registers. > + */ > + ret = cla_op_wait_lresp(dev, ®s->lresp); > + if (ret) > + return ret; > + > + for (i = 0; i < CLA_NUM_DATA_REGS; i++) > + regs->data[i] = cla_reg_read(dev, CLA_REG_DATA(i)); > + > + cla_for_each_accid(dev, accid) { > + ret = cla_regs_save_accel(dev, accid, regs, ®state_off); > + if (ret) > + return ret; > + } > + > + regs->accel_valid = true; [Severity: High] Does this leave a partially overwritten context state marked as valid on error? If cla_regs_save_accel() fails for an accelerator (e.g., due to a hardware timeout), the function returns the error immediately. Because regs->accel_valid is not cleared to false at the start of the function, it retains its prior value. If the context was previously successfully saved (regs->accel_valid == true), it remains marked as valid, despite the buffer now holding a torn, inconsistent mix of old state and newly overwritten partial data. On the next context switch, cla_regs_install_pl0() will see accel_valid == true and silently restore this corrupted partial state into the hardware, leading to unpredictable hardware behavior and data corruption. > + > + /* > + * "If the accelerator is non idle when ENTERSR is launched, then a [ ... ] > +static int cla_regs_install_pl0(struct cla_dev *dev, struct cla_ctx *ctx, > + unsigned int ctx_id) > +{ > + int i; > + int ret; > + unsigned int accid; > + size_t regstate_off = 0; > + struct cla_regs_pl0 *regs = ctx->regs[ctx_id]; [Severity: High] As with cla_regs_uninstall_pl0() above, does this code also index the ctx->regs array with the wrong identifier? Using ctx_id instead of the device index will cause an out-of-bounds array access or access the wrong device's register array. > + > + cla_for_each_accid(dev, accid) { [ ... ] > +void cla_ctx_free_regs(struct cla_domain *domain, struct cla_ctx *ctx) > +{ > + int i; > + > + for (i = 0; i < domain->nr_devs; i++) > + kvfree(ctx->regs[i]); [Severity: High] Can this code cause a NULL pointer dereference? In cla_ctx_alloc_regs(), if the initial memory allocation for regs_ptrs fails, the function returns -ENOMEM without assigning a value to ctx->regs. Assuming ctx was zero-initialized by the caller, ctx->regs remains NULL. When the caller handles this failure by invoking this common teardown path, it iterates and unconditionally executes kvfree(ctx->regs[i]). Since ctx->regs is NULL, array indexing results in a NULL pointer dereference. > + kfree(ctx->regs); > +} -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=6
