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 bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (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 D534BCA5FC1 for ; Wed, 30 Sep 2026 08:45:23 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:Content-Transfer-Encoding: Content-Type:In-Reply-To:From:References:Cc:To:Subject:MIME-Version:Date: Message-ID:Reply-To:Content-ID:Content-Description:Resent-Date:Resent-From: Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=k+2YE3/D0TMMc1GDR8Xa69ENbBbITfMmMCm6CARPooE=; b=Ldv/pEdBHlq5x7yhDusRQDin0I c/k3CQjWT/EIt6Ec27f8nV4AX4t1JfDBSY661V8kuDc7hHyORr6nRKTpKVwAxc+z75hy8Luljw5Ww gbtqHzVj/FRkmkP1iVGbNH81o9dJ5MxQowmhOXNBjS6W5qfFjsDu6PNJI1YYVCGMvb+d5SgEmvDLz w2NQJUQTCyCIhUps7SIQqTEnZU2wSlx4GAaT4DFEJ23h7zAwk6pN64WYWWXF5iq7r/Kae/UD/FNBu 0RS/xWlaMO44UxLSfmtmOWjsDoRVRPjWYxQcmUe9agbw3Yq7JNoVt+rl3nIhXybhJ639OxciSgeHD SxlMFCGw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.99.1 #2 (Red Hat Linux)) id 1xBpwC-00000005TNd-1pQL; Wed, 30 Sep 2026 08:45:16 +0000 Received: from desiato.infradead.org ([2001:8b0:10b:1:d65d:64ff:fe57:4e05]) by bombadil.infradead.org with esmtps (Exim 4.99.1 #2 (Red Hat Linux)) id 1xBpwA-00000005TMn-1fO4 for linux-arm-kernel@bombadil.infradead.org; Wed, 30 Sep 2026 08:45:14 +0000 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=infradead.org; s=desiato.20200630; h=Content-Transfer-Encoding:Content-Type :In-Reply-To:From:References:Cc:To:Subject:MIME-Version:Date:Message-ID: Sender:Reply-To:Content-ID:Content-Description; bh=k+2YE3/D0TMMc1GDR8Xa69ENbBbITfMmMCm6CARPooE=; b=IGLVMLSrkabPpFGxUKtNMMlinl Fi9YAhysMn196NLKkuDTZVyz3RUMAamnifuJU6yDThs412yD0+i1CMJ2X+KDWVU/lE62orjta51Dk ieI5uHIJf43/l/6Bt8zdhXhryUTWmTDVzvdY8hbjiFYdjv1y9kSWiRO5oOyIxPKnJJ5496TahM7OZ vK0Tj27wShsG2f6MBNHliofXQT0KhQy4fj0KArkyXKV12djzW3AXJ9d+1+fCc/iPp8G+++3+by5qF Hx0Aax4UV3i2K9En+ssDSbINida8WyfD4wrnVrlrqI7Mc4PKmtEovZE0VQxZcynpfSFfta5mhyXtQ zAdgCYnQ==; Received: from foss.arm.com ([217.140.110.172]) by desiato.infradead.org with esmtp (Exim 4.99.2 #2 (Red Hat Linux)) id 1xBpw4-00000003XX6-0PEu for linux-arm-kernel@lists.infradead.org; Wed, 30 Sep 2026 08:45:12 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 6BC55143D; Wed, 30 Sep 2026 01:45:02 -0700 (PDT) Received: from [10.57.9.178] (unknown [10.57.9.178]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 7877D3F85F; Wed, 30 Sep 2026 01:45:04 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=arm.com; s=foss; t=1790757905; bh=LlMfh49OrB202DnvA3FcBRcY8l4AnsgG8bcahu0coJM=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=WQXyV1lKlTfAuVQ/7PtouFRJbl6QFhh4roDvpHBFTRl3fY0l91NduM3AJa5rng0Hj S09u5UgbkNijosE/+5DmPbt6Ybdc5JLXmVj0sQEGRJNOZW71JSJDQJXkNUPs5AZPL4 +fV4DEgrv8LL8lkRfynpk7n5h174mniSCXLbAXa8= Message-ID: Date: Wed, 30 Sep 2026 09:45:03 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v20 4/9] firmware: arm_rmm: Add support for SRO Content-Language: en-GB To: sashiko-reviews@lists.linux.dev Cc: Oliver Upton , kvmarm@lists.linux.dev, kvm@vger.kernel.org, Marc Zyngier , Catalin Marinas , "linux-arm-kernel@lists.infradead.org" , Gavin Shan , Jonathan Cameron , Steven Price References: <20260929221623.1342076-1-suzuki.poulose@arm.com> <20260929221623.1342076-5-suzuki.poulose@arm.com> <20260929223042.681D61F000FF@smtp.kernel.org> From: Suzuki K Poulose In-Reply-To: <20260929223042.681D61F000FF@smtp.kernel.org> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.9.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20260930_094509_258482_AE2F99CC X-CRM114-Status: GOOD ( 28.97 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Hi, Responses inline. On 29/09/2026 23:30, sashiko-bot@kernel.org wrote: > Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: > - [Medium] arm_rmm: SRO cancellation discards partial progress limit leading to memory corruption > - [Medium] arm_rmm: integer overflow in granule clamp bypasses RMM protection > - [Medium] arm_rmm: missing validation of block_size_fld leads to division by zero > -- > > commit 44837285c82689401d14180cb932077aa7982b67 > Author: Steven Price > > firmware: arm_rmm: Add support for SRO > > This commit introduces support for Stateful RMI Operations (SRO), allowing RMI > commands to return incomplete and be resumed. It adds SRO state tracking, > memory donation and reclaim handling, and wrappers for SRO commands. > >> --- a/drivers/firmware/arm_rmm/rmi.c >> +++ b/drivers/firmware/arm_rmm/rmi.c >> @@ -14,6 +15,687 @@ >> +static long rmi_granule_range_delegate(unsigned long base, >> + unsigned long top, >> + unsigned long *out_top) >> +{ >> + struct arm_smccc_1_2_regs regs = { >> + SMC_RMI_GRANULE_RANGE_DELEGATE, base, top >> + }; >> + long ret = rmi_sro_execute(®s); >> + >> + if (ret == RMI_SUCCESS && out_top) >> + *out_top = regs.a1; >> + >> + return ret; >> +} > [ ... ] >> +static int rmi_sro_donate_contig(struct rmi_sro_state *sro, >> + unsigned long sro_handle, >> + unsigned long donatereq, >> + struct arm_smccc_1_2_regs *out_regs, >> + gfp_t gfp) >> +{ > [ ... ] >> +mem_donate: >> + rmi_op_mem_donate(sro_handle, >> + virt_to_phys(&sro->addr_list[sro->addr_count]), 1, >> + 0, out_regs); >> + donated_size = out_regs->a1 << PAGE_SHIFT; >> + >> + if (WARN_ON(donated_size > size)) >> + donated_size = size; > > [Severity: Medium] > Is there a risk of an integer overflow here before the bounds check? > > If a buggy or compromised RMM returns a maliciously large value for > out_regs->a1, the left shift by PAGE_SHIFT could overflow the 64-bit > donated_size variable. This would result in a small value that bypasses > the WARN_ON() check. Fair point, this was due to the removal of donated_granules. I will rectify this. > > The host kernel might then erroneously believe the RMM consumed fewer > granules than it actually did, freeing the remaining memory back to the > allocator while the RMM is actively using it. This could result in memory > corruption. > > [ ... ] >> +static int rmi_sro_donate_noncontig(struct rmi_sro_state *sro, >> + unsigned long sro_handle, >> + unsigned long donatereq, >> + struct arm_smccc_1_2_regs *out_regs, >> + gfp_t gfp) >> +{ >> + unsigned long block_size_fld = RMI_DONATE_BLOCK_SIZE(donatereq); >> + unsigned long block_size = rmi_addr_block_size_to_bytes(block_size_fld); >> + unsigned long count = RMI_DONATE_COUNT(donatereq); >> + unsigned long state = RMI_DONATE_STATE(donatereq); >> + unsigned long found = 0; >> + unsigned long donated_granules; >> + unsigned long granules_per_block = block_size >> PAGE_SHIFT; >> + unsigned long consumed_blocks; > [ ... ] >> + donated_granules = out_regs->a1; > [ ... ] >> + consumed_blocks = donated_granules / granules_per_block; > > [Severity: Medium] > Could an unvalidated block_size_fld lead to a division by zero panic here? > > While count and state are explicitly validated against anomalies, > block_size_fld from the firmware appears to be blindly trusted. If the > firmware returns an out-of-bounds value (e.g., 6), the shift amount in > rmi_addr_block_size_to_bytes() can evaluate modulo 64, yielding a block_size > smaller than PAGE_SIZE. This is not possible. The field is a 2bit field and cannot be greater than 3. See the RMI_DONATE_BLOCK_SIZE() > > This would cause granules_per_block to evaluate to 0, triggering a kernel > panic when donated_granules is divided by granules_per_block. > > [ ... ] >> +long rmi_sro_execute(struct arm_smccc_1_2_regs *regs) >> +{ >> + bool cancelled = false; >> + unsigned long sro_handle; > [ ... ] >> + if (cancelled) >> + return -ECANCELED; >> + >> + return regs->a0; >> +} > > [Severity: Medium] > When an operation is cancelled, does this unconditionally discard the > output registers containing the partial progress limit? > > If a buggy or compromised RMM returns an unexpected MEMREQ during a non-memory > SRO like SMC_RMI_GRANULE_RANGE_DELEGATE, rmi_sro_execute() issues an > RMI_OP_CANCEL and unconditionally returns -ECANCELED. This drops the final > state in the a1 register. We already WARN_ON_ONCE(1), when we get a memreq in a non-memory transferring SRO operation. There is nothing we could do in this case from the generic helper, than to inform the caller that we had to cancel it. This is a fundamental misbehavior from an RMM implementation. There are other ways the RMM could crash the host. e.g., Don't really undelegate while reporting success. So, I wouldn't care about such pathological cases. Cheers Suzuki > > As a result, callers like rmi_granule_range_delegate() fail to capture > the progress limit. The kernel would then undelegate fewer granules > than the RMM successfully processed before returning them to the normal > allocator, which can lead to fatal Granule Protection Faults when the host > later accesses them. >