Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] Heap buffer overflow due to unit mismatch in internal accelerator state (IAS) memory allocation and bounds checking. - [Medium] Incomplete context save leaves `regs->accel_valid` as true, leading to corrupted hardware state on subsequent restore. --
--- Patch [5]: [RFC PATCH v1 5/8] misc/arm-cla: Accelerator context save and restore --- commit 96459db186d7c9fdaf43a3521dada1e5846d6b74 Author: Jean-Philippe Brucker <[email protected]> misc/arm-cla: Accelerator context save and restore This commit adds helpers to save and restore CLA context for later domain switching. The saved state includes DATA registers, LRESP, per-accelerator SRSTATE, and 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..7507796cd551f > --- /dev/null > +++ b/drivers/misc/arm-cla/cla-regs.c > @@ -0,0 +1,263 @@ [ ... ] > +static int cla_regs_save_accel(struct cla_dev *dev, unsigned int accid, > + struct cla_regs *regs, off_t *regstate_off) > +{ [ ... ] > + regstate_size = FIELD_GET(CLA_SRSTATE_0_REGSTATE, srstate[0]); > + if (WARN_ON(*regstate_off + regstate_size * 8 > dev->iassizes)) > + return -ENOSPC; [Severity: High] Does this bounds check correctly handle the units for *regstate_off? *regstate_off is incremented by regstate_size (which is a register count), so it is also a register count. If dev->iassizes is in bytes, this check improperly adds a register count (*regstate_off) to a byte count (regstate_size * 8) and compares it against a byte count. Because regs->regstate + *regstate_off scales the offset by 8 during the memory access, this check can pass even when the access goes out of bounds. For example, if dev->iassizes = 160 bytes, *regstate_off = 20, and regstate_size = 10, the check evaluates to 20 + 80 <= 160 (passes). However, the accessed memory is at byte offset (20 + 10) * 8 = 240, overflowing the buffer. > + > + if (regstate_size) { > + ret = cla_op_regread(dev, accid, CLA_REG_IASn, regstate_size, > + regs->regstate + *regstate_off); [ ... ] > +static int cla_regs_restore_accel(struct cla_dev *dev, unsigned int accid, > + struct cla_regs *regs, off_t *regstate_off) > +{ [ ... ] > + regstate_size = FIELD_GET(CLA_SRSTATE_0_REGSTATE, srstate[0]); > + if (WARN_ON(*regstate_off + regstate_size * 8 > dev->iassizes)) > + return -ENOSPC; [Severity: High] This appears to have the exact same unit mismatch issue as cla_regs_save_accel(), which could lead to out-of-bounds memory reads when restoring accelerator state. > + > + if (regstate_size) { > + ret = cla_op_regwrite(dev, accid, CLA_REG_IASn, regstate_size, > + regs->regstate + *regstate_off); [ ... ] > +int cla_regs_switch_out(struct cla_dev *dev, struct cla_regs *regs, > + bool save_regs) > +{ [ ... ] > + if (save_regs) { > + 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; [Severity: Medium] If cla_regs_save_accel() fails here, the function returns without clearing regs->accel_valid. Since regs is allocated once per domain and reused, accel_valid might already be true from a previous successful save. Returning an error here leaves the structure partially overwritten (a mix of old and new accelerator states) but still marked as valid. If the caller handles this switch-out failure non-fatally and retries the context later, will cla_regs_switch_in() restore this corrupted state to the hardware? > + } > + > + regs->accel_valid = true; > + } [ ... ] > +struct cla_regs **cla_regs_alloc_domain(struct cla_domain *domain) > +{ [ ... ] > + for (i = 0; i < domain->nr_devs; i++) { > + struct cla_regs *regs; > + struct cla_dev *dev = domain->devs[i]; > + > + /* > + * The regs structures are only ever accessed from the CLA > + * device's CPU, so try to allocate them on the right NUMA node > + */ > + size = sizeof(*regs) + dev->iassizes; [Severity: High] If dev->iassizes represents a register count rather than a byte count, should it be scaled by sizeof(u64) here? If it is unscaled, the allocation for the flexible array regs->regstate will be 8 times too small, leading to heap buffer overflows during save and restore operations. > + regs = kvzalloc_node(size, GFP_KERNEL_ACCOUNT, > + cpu_to_node(dev->cpu)); -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=5
