linux-kernel - Re: [PATCH v2 07/11] coco: guest: arm64: Validate Realm MMIO mappings from TDISP report

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: <20251120174359.00001817@huawei.com>
Date: Thu, 20 Nov 2025 17:43:59 +0000
From: Jonathan Cameron <jonathan.cameron@...wei.com>
To: "Aneesh Kumar K.V (Arm)" <aneesh.kumar@...nel.org>
CC: <linux-coco@...ts.linux.dev>, <kvmarm@...ts.linux.dev>,
	<linux-pci@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
	<dan.j.williams@...el.com>, <aik@....com>, <lukas@...ner.de>, Samuel Ortiz
	<sameo@...osinc.com>, Xu Yilun <yilun.xu@...ux.intel.com>, Jason Gunthorpe
	<jgg@...pe.ca>, Suzuki K Poulose <Suzuki.Poulose@....com>, Steven Price
	<steven.price@....com>, Bjorn Helgaas <helgaas@...nel.org>, Catalin Marinas
	<catalin.marinas@....com>, Marc Zyngier <maz@...nel.org>, Will Deacon
	<will@...nel.org>, Oliver Upton <oliver.upton@...ux.dev>
Subject: Re: [PATCH v2 07/11] coco: guest: arm64: Validate Realm MMIO
 mappings from TDISP report

On Mon, 17 Nov 2025 19:30:03 +0530
"Aneesh Kumar K.V (Arm)" <aneesh.kumar@...nel.org> wrote:

> Parse the TDISP device interface report and drive the RSI
> RDEV_VALIDATE_MAPPING handshake for each Realm MMIO window. The new
> helper walks the reported ranges, rejects malformed entries, and either
> validates the IPA->PA mapping when the device transitions to RUN or tears
> it down with RIPAS updates on unlock.
> 
> Signed-off-by: Aneesh Kumar K.V (Arm) <aneesh.kumar@...nel.org>

A few minor things in here.

> diff --git a/drivers/virt/coco/arm-cca-guest/rsi-da.c b/drivers/virt/coco/arm-cca-guest/rsi-da.c
> index aa6e13e4c0ea..c70fb7dd4838 100644
> --- a/drivers/virt/coco/arm-cca-guest/rsi-da.c
> +++ b/drivers/virt/coco/arm-cca-guest/rsi-da.c

> +int cca_apply_interface_report_mappings(struct pci_dev *pdev, bool validate)
> +{
> +	int ret;
> +	struct resource *r;
> +	unsigned int range_id;
> +	phys_addr_t mmio_start_phys;
> +	struct pci_tdisp_mmio_range *mmio_range;
> +	phys_addr_t ipa_start, ipa_end, bar_offset;
> +	struct pci_tdisp_device_interface_report *interface_report;
> +	struct cca_guest_dsc *dsc = to_cca_guest_dsc(pdev);
> +
> +	interface_report = (struct pci_tdisp_device_interface_report *)dsc->interface_report;
> +	mmio_range = (struct pci_tdisp_mmio_range *)(interface_report + 1);
> +
> +
> +	for (int i = 0; i < interface_report->mmio_range_count; i++, mmio_range++) {
> +

I would make scope of some of the variables used in here explicit by declaring them in the loop.
e.g. range_id.

> +		range_id = FIELD_GET(TSM_INTF_REPORT_MMIO_RANGE_ID, mmio_range->range_attributes);
> +
I would drop this blank line. Keep the variable set and error check tightly together.
> +		if (range_id >= PCI_NUM_RESOURCES) {
> +			pci_warn(pdev, "Skipping broken range [%d] #%d %d\n",
> +				 i, range_id, mmio_range->num_pages);
> +			continue;
> +		}
> +
> +		r = pci_resource_n(pdev, range_id);
> +

Likewise I would drop this blank line.

> +		if (r->end == r->start ||
> +		    resource_size(r) & ~PAGE_MASK || !mmio_range->num_pages) {
Seems like an odd wrap.
		if (r->end == r->start ||
		    resource_size(r) & ~PAGE_MASK ||
		    !mmio_range->num_pages) {

or
		if (r->end == r->start || resource_size(r) & ~PAGE_MASK ||
		    !mmio_range->num_pages) {

Only exception being if you are going to edit this in a patch soon after and this
is about avoiding churn in that patch  - if so ignore this comment.


> +			pci_warn(pdev, "Skipping broken range [%d] #%d %d pages, %llx..%llx\n",
> +				i, range_id, mmio_range->num_pages, r->start, r->end);
> +			continue;
> +		}
> +
> +		if (FIELD_GET(TSM_INTF_REPORT_MMIO_IS_NON_TEE, mmio_range->range_attributes)) {
> +			pci_info(pdev, "Skipping non-TEE range [%d] #%d %d pages, %llx..%llx\n",
> +				 i, range_id, mmio_range->num_pages, r->start, r->end);
> +			continue;
> +		}
> +
> +		/* No secure interrupts, we should not find this set, ignore for now. */
> +		if (FIELD_GET(TSM_INTF_REPORT_MMIO_MSIX_TABLE, mmio_range->range_attributes) ||
> +		    FIELD_GET(TSM_INTF_REPORT_MMIO_PBA, mmio_range->range_attributes)) {
> +			pci_info(pdev, "Skipping MSIX (%ld/%ld) range [%d] #%d %d pages, %llx..%llx\n",
> +				 FIELD_GET(TSM_INTF_REPORT_MMIO_MSIX_TABLE, mmio_range->range_attributes),
> +				 FIELD_GET(TSM_INTF_REPORT_MMIO_PBA, mmio_range->range_attributes),
> +				 i, range_id, mmio_range->num_pages, r->start, r->end);
> +			continue;
> +		}
> +
> +		/* units in 4K size*/
> +		mmio_start_phys = mmio_range->first_page << 12;
> +		bar_offset = mmio_start_phys & (pci_resource_len(pdev, range_id) - 1);
> +		ipa_start = r->start + bar_offset;
> +		ipa_end = ipa_start + (mmio_range->num_pages << 12);
> +
> +		if (!validate)
> +			ret = rsi_invalidate_dev_mapping(ipa_start, ipa_end);
> +		if (ret)
> +			return ret;
> +	}
> +	return 0;
> +}
> diff --git a/drivers/virt/coco/arm-cca-guest/rsi-da.h b/drivers/virt/coco/arm-cca-guest/rsi-da.h
> index fa9cc01095da..32cf90beb55e 100644
> --- a/drivers/virt/coco/arm-cca-guest/rsi-da.h
> +++ b/drivers/virt/coco/arm-cca-guest/rsi-da.h
> @@ -12,6 +12,28 @@

>  struct cca_guest_dsc {
>  	struct pci_tsm_devsec pci;
>  	void *interface_report;
> @@ -42,5 +64,5 @@ int cca_device_unlock(struct pci_dev *pdev);
>  int cca_update_device_object_cache(struct pci_dev *pdev, struct cca_guest_dsc *dsc);
>  struct page *alloc_shared_pages(int nid, gfp_t gfp_mask, unsigned long min_size);
>  int free_shared_pages(struct page *page, unsigned long min_size);
> -

Check all patches for noise like this. Either that white space is good to have in which case
keep it. Or it's not in which case delete.

> +int cca_apply_interface_report_mappings(struct pci_dev *pdev, bool validate);
>  #endif