| 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
| ||
|
Message-ID: <8f57eb6c-d1da-4638-9b3e-a159dbed9624@linux.intel.com>
Date: Mon, 1 Jul 2024 13:26:05 +0800
From: Baolu Lu <baolu.lu@...ux.intel.com>
To: Jason Gunthorpe <jgg@...pe.ca>
Cc: baolu.lu@...ux.intel.com, Kevin Tian <kevin.tian@...el.com>,
Joerg Roedel <joro@...tes.org>, Will Deacon <will@...nel.org>,
Robin Murphy <robin.murphy@....com>,
Jean-Philippe Brucker <jean-philippe@...aro.org>,
Nicolin Chen <nicolinc@...dia.com>, Yi Liu <yi.l.liu@...el.com>,
Jacob Pan <jacob.jun.pan@...ux.intel.com>,
Joel Granados <j.granados@...sung.com>, iommu@...ts.linux.dev,
virtualization@...ts.linux-foundation.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v7 08/10] iommufd: Associate fault object with
iommufd_hw_pgtable
On 2024/6/29 6:13, Jason Gunthorpe wrote:
> On Sun, Jun 16, 2024 at 02:11:53PM +0800, Lu Baolu wrote:
>
>> @@ -308,13 +315,29 @@ int iommufd_hwpt_alloc(struct iommufd_ucmd *ucmd)
>> goto out_put_pt;
>> }
>>
>> + if (cmd->flags & IOMMU_HWPT_FAULT_ID_VALID) {
>> + struct iommufd_fault *fault;
>> +
>> + fault = iommufd_get_fault(ucmd, cmd->fault_id);
>> + if (IS_ERR(fault)) {
>> + rc = PTR_ERR(fault);
>> + goto out_hwpt;
>> + }
>> + hwpt->fault = fault;
>> + hwpt->domain->iopf_handler = iommufd_fault_iopf_handler;
>> + hwpt->domain->fault_data = hwpt;
>
> This is not the right refcounting for a longterm reference... The PT
> above shows the pattern:
>
> pt_obj = iommufd_get_object(ucmd->ictx, cmd->pt_id, IOMMUFD_OBJ_ANY);
> hwpt_paging = iommufd_hwpt_paging_alloc()
> refcount_inc(&ioas->obj.users);
>
> iommufd_put_object(ucmd->ictx, pt_obj);
>
> Which is to say you need to incr users and then do the put object. And
> iommufd_object_abort_and_destroy() will always destroy the ref on the
> fault if the fault is non-null so the error handling will double free.
>
> fail_nth is intended to catch this, but you have to add enough inputs
> to cover the new cases when you add them, it seems like that is
> missing in this series. ie add a fault object and hwpt alloc to a
> fail_nth test and see we execute the iommufd_ucmd_respond() failure
> path.
Yes. I will add below fail_nth case:
--- a/tools/testing/selftests/iommu/iommufd_utils.h
+++ b/tools/testing/selftests/iommu/iommufd_utils.h
@@ -201,6 +201,12 @@ static int _test_cmd_hwpt_alloc(int fd, __u32
device_id, __u32 pt_id, __u32 ft_i
ASSERT_EQ(0, _test_cmd_hwpt_alloc(self->fd, device_id, pt_id,
fault_id, \
flags, hwpt_id, data_type,
data, \
data_len))
+#define test_err_hwpt_alloc_iopf(_errno, device_id, pt_id, fault_id,
flags, \
+ hwpt_id, data_type, data, data_len)
\
+ EXPECT_ERRNO(_errno,
\
+ _test_cmd_hwpt_alloc(self->fd, device_id, pt_id,
fault_id, \
+ flags, hwpt_id, data_type,
data, \
+ data_len))
#define test_cmd_hwpt_check_iotlb(hwpt_id, iotlb_id, expected)
\
({
\
diff --git a/tools/testing/selftests/iommu/iommufd.c
b/tools/testing/selftests/iommu/iommufd.c
index 5b0169875a4d..93634e53e95e 100644
--- a/tools/testing/selftests/iommu/iommufd.c
+++ b/tools/testing/selftests/iommu/iommufd.c
@@ -338,6 +338,10 @@ TEST_F(iommufd_ioas, alloc_hwpt_nested)
&nested_hwpt_id[1],
IOMMU_HWPT_DATA_SELFTEST, &data,
sizeof(data));
+ test_err_hwpt_alloc_iopf(ENOENT, self->device_id,
parent_hwpt_id,
+ UINT32_MAX,
IOMMU_HWPT_FAULT_ID_VALID,
+ &iopf_hwpt_id,
IOMMU_HWPT_DATA_SELFTEST,
+ &data, sizeof(data));
test_cmd_hwpt_alloc_iopf(self->device_id,
parent_hwpt_id, fault_id,
IOMMU_HWPT_FAULT_ID_VALID,
&iopf_hwpt_id,
IOMMU_HWPT_DATA_SELFTEST, &data,
>
> --- a/drivers/iommu/iommufd/hw_pagetable.c
> +++ b/drivers/iommu/iommufd/hw_pagetable.c
> @@ -14,7 +14,7 @@ static void __iommufd_hwpt_destroy(struct iommufd_hw_pagetable *hwpt)
> iommu_domain_free(hwpt->domain);
>
> if (hwpt->fault)
> - iommufd_put_object(hwpt->fault->ictx, &hwpt->fault->obj);
> + refcount_dec(&hwpt->fault->obj.users);
> }
>
> void iommufd_hwpt_paging_destroy(struct iommufd_object *obj)
> @@ -326,18 +326,17 @@ int iommufd_hwpt_alloc(struct iommufd_ucmd *ucmd)
> hwpt->fault = fault;
> hwpt->domain->iopf_handler = iommufd_fault_iopf_handler;
> hwpt->domain->fault_data = hwpt;
> + refcount_inc(&fault->obj.users);
> + iommufd_put_object(ucmd->ictx, &fault->obj);
> }
>
> cmd->out_hwpt_id = hwpt->obj.id;
> rc = iommufd_ucmd_respond(ucmd, sizeof(*cmd));
> if (rc)
> - goto out_put_fault;
> + goto out_hwpt;
> iommufd_object_finalize(ucmd->ictx, &hwpt->obj);
> goto out_unlock;
>
> -out_put_fault:
> - if (cmd->flags & IOMMU_HWPT_FAULT_ID_VALID)
> - iommufd_put_object(ucmd->ictx, &hwpt->fault->obj);
> out_hwpt:
> iommufd_object_abort_and_destroy(ucmd->ictx, &hwpt->obj);
> out_unlock:
>
.. and merge above change to this patch.
Best regards,
baolu
Powered by blists - more mailing lists