From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 26334CC6B33 for ; Thu, 15 Oct 2026 14:41:56 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 4510A112394; Thu, 15 Oct 2026 14:41:50 +0000 (UTC) Received: from sea.source.kernel.org (sea.source.kernel.org [172.234.252.31]) by gabe.freedesktop.org (Postfix) with ESMTPS id D306F112389 for ; Thu, 15 Oct 2026 14:41:43 +0000 (UTC) Received: from smtp.kernel.org (quasi.space.kernel.org [100.103.45.18]) by sea.source.kernel.org (Postfix) with ESMTP id BC19D43C3E; Thu, 15 Oct 2026 14:41:43 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 30D761F000C4; Thu, 15 Oct 2026 14:41:43 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=kernel.org; s=k20260515; t=1792075303; bh=+kqI9L/9ctD2NK6q2Ru9phtpBLvNfgJg6s/BMhF22XY=; h=From:Subject:Reply-To:To:Cc:In-Reply-To:References:Date; b=d5IyUW0jJ6PjZnI6j0LpyGQxn33QfKiLP6rIMxnnpEvl/BSKc/TgY8c/0M0WVibrW WxX5O9aHcGH0/RumIowBi5muvdXkEsKp79a6c1Twacea40HcPmM3u5g3BYPv4LAb9K c/bITBMAEL2KehAsz8fKO1m4tcwDQ1Keku2CGJFmRsX4UPDS1lay1xJC0ZCDUz1vBf KQz1TEzlU/J+YYyCcArrXDv5yzS2rS9e1GQQCwh91p47sgIQRy6COQA8fsSfpsWLnM 49BhZ27EYI+S7lURgZcaGSKOd0l6fazraRr/55pOxWEpoCubB/tHrq1DAA10cocCf8 Bb1Kc4llFH7VQ== Message-ID: From: sashiko-bot@kernel.org Subject: Re: [RFC PATCH v2 06/15] misc/arm-cla: Accelerator context save and restore To: "Ryan Roberts" Cc: kvmarm@lists.linux.dev, kvm@vger.kernel.org, devicetree@vger.kernel.org, dri-devel@lists.freedesktop.org, robh@kernel.org, conor+dt@kernel.org, "Oliver Upton" , "Marc Zyngier" In-Reply-To: <20261015142552.4128028-7-ryan.roberts@arm.com> References: <20261015142552.4128028-1-ryan.roberts@arm.com> <20261015142552.4128028-7-ryan.roberts@arm.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Date: Thu, 15 Oct 2026 14:41:42 +0000 X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: sashiko-reviews@lists.linux.dev Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" 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 e= rror - [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 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-r= egs.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 =3D 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 =3D 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 breaki= ng 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 *c= tx, > + unsigned int ctx_id) > +{ > + int i; > + int ret; > + unsigned int accid; > + size_t regstate_off =3D 0; > + struct cla_regs_pl0 *regs =3D 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_dev= s, 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 >=3D 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 ind= ex 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 =3D cla_op_wait_lresp(dev, ®s->lresp); > + if (ret) > + return ret; > + > + for (i =3D 0; i < CLA_NUM_DATA_REGS; i++) > + regs->data[i] =3D cla_reg_read(dev, CLA_REG_DATA(i)); > + > + cla_for_each_accid(dev, accid) { > + ret =3D cla_regs_save_accel(dev, accid, regs, ®state_off); > + if (ret) > + return ret; > + } > + > + regs->accel_valid =3D true; [Severity: High] Does this leave a partially overwritten context state marked as valid on er= ror? 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 =3D=3D = 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 =3D=3D true and silently restore this corrupted partial state i= nto 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 =3D 0; > + struct cla_regs_pl0 *regs =3D 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 =3D 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 fai= ls, the function returns -ENOMEM without assigning a value to ctx->regs. Assumi= ng 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); > +} --=20 Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20261015142552.4128= 028-1-ryan.roberts@arm.com?part=3D6