linux-kernel - Re: [PATCH v4 4/7] PCI: keystone: Add supported for PVU-based DMA isolation on AM654

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240905191635.GA395079@bhelgaas>
Date: Thu, 5 Sep 2024 14:16:35 -0500
From: Bjorn Helgaas <helgaas@...nel.org>
To: Jan Kiszka <jan.kiszka@...mens.com>
Cc: Nishanth Menon <nm@...com>, Santosh Shilimkar <ssantosh@...nel.org>,
	Vignesh Raghavendra <vigneshr@...com>,
	Tero Kristo <kristo@...nel.org>, Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	linux-pci@...r.kernel.org, Siddharth Vadapalli <s-vadapalli@...com>,
	Bao Cheng Su <baocheng.su@...mens.com>,
	Hua Qian Li <huaqian.li@...mens.com>,
	Diogo Ivo <diogo.ivo@...mens.com>,
	Lorenzo Pieralisi <lpieralisi@...nel.org>,
	Krzysztof Wilczyński <kw@...ux.com>,
	Bjorn Helgaas <bhelgaas@...gle.com>,
	Kishon Vijay Abraham I <kishon@...nel.org>
Subject: Re: [PATCH v4 4/7] PCI: keystone: Add supported for PVU-based DMA
 isolation on AM654

On Thu, Sep 05, 2024 at 09:07:36PM +0200, Jan Kiszka wrote:
> On 05.09.24 18:33, Bjorn Helgaas wrote:
> > [+cc Kishon, just in case you have time/interest ;)]
> > 
> > On Wed, Sep 04, 2024 at 12:00:13PM +0200, Jan Kiszka wrote:
> >> From: Jan Kiszka <jan.kiszka@...mens.com>
> >>
> >> The AM654 lacks an IOMMU, thus does not support isolating DMA requests
> >> from untrusted PCI devices to selected memory regions this way. Use
> >> static PVU-based protection instead.
> >>
> >> For this, we use the availability of restricted-dma-pool memory regions
> >> as trigger and register those as valid DMA targets with the PVU.
> > 
> > I guess the implication is that DMA *outside* the restricted-dma-pool
> > just gets dropped, and the Requester would see Completion Timeouts or
> > something for reads?
> 
> I cannot tell what happens on the PCI bus in that case, maybe someone 
> from TI can help out.
> 
> On the host side, the PVU will record an error and raise an interrupt 
> which will make the driver report that to the kernel log. That's quite 
> similar to what IOMMU drivers do on translation faults.

The main thing is that the DMA doesn't complete, as you mentioned
below.

> > Since there's no explicit use of "restricted-dma-pool" elsewhere in
> > this patch, I assume the setup above causes the controller to drop any
> > DMA accesses outside that pool?  I think a comment about how the
> > controller behavior is being changed would be useful.  Basically the
> > same comment as for the commit log.
> 
> Right, this is what will happen. Will add some comment.
> 
> > Would there be any value in a dmesg note about a restriction being
> > enforced?  Seems like it's dependent on both CONFIG_TI_PVU and some DT
> > properties, and since those are invisible in the log, maybe a note
> > would help understand/debug any issues?
> 
> This is what you will see when there are reserved region and PVU in 
> play:
> 
> keystone-pcie 5600000.pcie: assigned reserved memory node restricted-dma@...00000
> ti-pvu 30f80000.iommu: created TLB entry 0.2: 0xc0000000, psize 4 (0x02000000)
> ti-pvu 30f80000.iommu: created TLB entry 0.3: 0xc2000000, psize 4 (0x02000000)
> ...
> ath9k 0000:01:00.0: assigned reserved memory node restricted-dma@...00000

Looks reasonable and solves my concern.

> >> +	of_for_each_phandle(&it, err, pdev->dev.of_node, "memory-region",
> >> +			    NULL, 0) {
> >> +		if (of_device_is_compatible(it.node, "restricted-dma-pool") &&
> >> +		    of_address_to_resource(it.node, 0, &phys) == 0)
> >> +			ti_pvu_remove_region(KS_PCI_VIRTID, &phys);
> > 
> > I guess it's not important to undo the PCIE_VMAP_xP_CTRL_EN and
> > related setup that was done by ks_init_restricted_dma()?
> > 
> 
> Right, I didn't find a reason to do that.

OK, as long as you considered it :)

Bjorn