linux-kernel - Re: [PATCH RESEND STABLE v6.1] iommu: Avoid races around default domain allocations

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <2024032955-exponent-nativity-fba4@gregkh>
Date: Fri, 29 Mar 2024 11:21:06 +0100
From: Greg KH <gregkh@...uxfoundation.org>
To: Nikhil V <quic_nprakash@...cinc.com>
Cc: Charan Teja Kalla <quic_charante@...cinc.com>,
	Joerg Roedel <joro@...tes.org>, Will Deacon <will@...nel.org>,
	Robin Murphy <robin.murphy@....com>, linux-kernel@...r.kernel.org,
	iommu@...ts.linux.dev, stable@...r.kernel.org
Subject: Re: [PATCH RESEND STABLE v6.1] iommu: Avoid races around default
 domain allocations

On Tue, Mar 05, 2024 at 01:01:12PM +0530, Nikhil V wrote:
> 
> 
> On 3/4/2024 6:55 PM, Greg KH wrote:
> > On Mon, Mar 04, 2024 at 04:40:50PM +0530, Nikhil V wrote:
> > > From: Charan Teja Kalla <quic_charante@...cinc.com>
> > > 
> > > This fix is applicable for LTS kernel, 6.1.y. In latest kernels, this race
> > > issue is fixed by the patch series [1] and [2]. The right thing to do here
> > > would have been propagating these changes from latest kernel to the stable
> > > branch, 6.1.y. However, these changes seems too intrusive to be picked for
> > > stable branches. Hence, the fix proposed can be taken as an alternative
> > > instead of backporting the patch series.
> > > [1] https://lore.kernel.org/all/0-v8-81230027b2fa+9d-iommu_all_defdom_jgg@nvidia.com/
> > > [2] https://lore.kernel.org/all/0-v5-1b99ae392328+44574-iommu_err_unwind_jgg@nvidia.com/
> > > 
> > > Issue:
> > > A race condition is observed when arm_smmu_device_probe and
> > > modprobe of client devices happens in parallel. This results
> > > in the allocation of a new default domain for the iommu group
> > > even though it was previously allocated and the respective iova
> > > domain(iovad) was initialized. However, for this newly allocated
> > > default domain, iovad will not be initialized. As a result, for
> > > devices requesting dma allocations, this uninitialized iovad will
> > > be used, thereby causing NULL pointer dereference issue.
> > > 
> > > Flow:
> > > - During arm_smmu_device_probe, bus_iommu_probe() will be called
> > > as part of iommu_device_register(). This results in the device probe,
> > > __iommu_probe_device().
> > > 
> > > - When the modprobe of the client device happens in parallel, it
> > > sets up the DMA configuration for the device using of_dma_configure_id(),
> > > which inturn calls iommu_probe_device(). Later, default domain is
> > > allocated and attached using iommu_alloc_default_domain() and
> > > __iommu_attach_device() respectively. It then ends up initializing a
> > > mapping domain(IOVA domain) and rcaches for the device via
> > > arch_setup_dma_ops()->iommu_setup_dma_ops().
> > > 
> > > - Now, in the bus_iommu_probe() path, it again tries to allocate
> > > a default domain via probe_alloc_default_domain(). This results in
> > > allocating a new default domain(along with IOVA domain) via
> > > __iommu_domain_alloc(). However, this newly allocated IOVA domain
> > > will not be initialized.
> > > 
> > > - Now, when the same client device tries dma allocations via
> > > iommu_dma_alloc(), it ends up accessing the rcaches of the newly
> > > allocated IOVA domain, which is not initialized. This results
> > > into NULL pointer dereferencing.
> > > 
> > > Fix this issue by adding a check in probe_alloc_default_domain()
> > > to see if the iommu_group already has a default domain allocated
> > > and initialized.
> > > 
> > > Cc: <stable@...r.kernel.org> # see patch description, fix applicable only for 6.1.y
> > > Signed-off-by: Charan Teja Kalla <quic_charante@...cinc.com>
> > > Co-developed-by: Nikhil V <quic_nprakash@...cinc.com>
> > > Signed-off-by: Nikhil V <quic_nprakash@...cinc.com>
> > > ---
> > >   drivers/iommu/iommu.c | 3 +++
> > >   1 file changed, 3 insertions(+)
> > 
> > Why RESEND?  What happened to the first send?
> > 
> > thanks,
> > 
> > greg k-h
> 
> 
> Hi Greg,
> 
> There are no changes as such w.r.t first send, [1]. It is resent to gain
> attention on this patch. Also, we have added a proper Cc: stable tag with
> this patch.

Thanks, now queued up.

greg k-h