Re: [Syzkaller & bisect] There is WARNING in iopt_remove_access in upstream patch "iommufd/selftest: Add IOMMU_TEST_OP_ACCESS_REPLACE_IOAS coverage"

[Nicolin Chen <nicolinc@nvidia.com>]

Hi Pengfei,

On Thu, Jun 15, 2023 at 10:23:09AM +0800, Pengfei Xu wrote:

> Hi Nicolin,
> 
> Greeting!
> 
> There is WARNING in iopt_remove_access in related patch:
> https://lore.kernel.org/lkml/e93964b04d5b0f45344931fcae0e8696dd649988.1683593831.git.nicolinc@nvidia.com/#t
> 
> I tested Intel internal kernel and syzkaller found this issue by accident,
> I checked that internal commit:"e93964b04d5b iommufd/selftest: Add
> IOMMU_TEST_OP_ACCESS_REPLACE_IOAS coverage" was same as above link patch.
> 
> It seems that syzkaller accidentally filled the syscall mutating parameter
> during a long fuzzing time and discovered this issue:
> " *(uint32_t*)0x20000004 = 0xb; // IOMMU_TEST_OP_ACCESS_REPLACE_IOAS=0xb"
> https://github.com/xupengfe/syzkaller_logs/blob/210a8d4069655735cc2bc2756981a944857a734c/230614_070652_iopt_remove_access/repro.c#LL187C3-L187C32
> 
> All analysis and detailed info: https://github.com/xupengfe/syzkaller_logs/tree/main/230614_070652_iopt_remove_access
> Syzkaller reproduced code: https://github.com/xupengfe/syzkaller_logs/blob/main/230614_070652_iopt_remove_access/repro.c
> Syzkaller syscall reproduced steps: https://github.com/xupengfe/syzkaller_logs/blob/main/230614_070652_iopt_remove_access/repro.prog
> Kconfig: https://raw.githubusercontent.com/xupengfe/syzkaller_logs/main/230614_070652_iopt_remove_access/kconfig_origin
> Bisect info: https://github.com/xupengfe/syzkaller_logs/blob/main/230614_070652_iopt_remove_access/bisect_info.log
> Reproduced bzimage: https://github.com/xupengfe/syzkaller_logs/blob/main/230614_070652_iopt_remove_access/bzImage_e93964b04d5b0f45344931fcae0e8696dd649988.xz
> e93964b04d5b reproduced dmesg: https://raw.githubusercontent.com/xupengfe/syzkaller_logs/main/230614_070652_iopt_remove_access/e93964b04d5b0f45344931fcae0e8696dd64998_dmesg.log
> 
> I hope it's helpful.

Thanks for the report!

It turns out to be a bug in the new iommufd_access_change_pt()
that does iopt_add_access() prior to __iommufd_access_detach().
However, iopt_add_access() overrides access->iopt_access_list_id
being read by the following __iommufd_access_detach(). Thus, it
triggers the WARNING.

A fix could be like this (will integrate in the next version)
-------------------------------------------------------------------------------
diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
index a106f7c655d6..98fab19b92b9 100644
--- a/drivers/iommu/iommufd/device.c
+++ b/drivers/iommu/iommufd/device.c
@@ -796,6 +796,7 @@ EXPORT_SYMBOL_NS_GPL(iommufd_access_detach, IOMMUFD);

 static int iommufd_access_change_pt(struct iommufd_access *access, u32 ioas_id)
 {
+       struct iommufd_ioas *cur_ioas = access->ioas;
        struct iommufd_ioas *new_ioas;
        int rc;

@@ -805,15 +806,20 @@ static int iommufd_access_change_pt(struct iommufd_access *access, u32 ioas_id)
        if (IS_ERR(new_ioas))
                return PTR_ERR(new_ioas);

+       if (cur_ioas)
+               __iommufd_access_detach(access);
+
        rc = iopt_add_access(&new_ioas->iopt, access);
        if (rc) {
                iommufd_put_object(&new_ioas->obj);
+               if (cur_ioas) {
+                       WARN_ON(iommufd_access_change_pt(access,
+                                                        cur_ioas->obj.id));
+               }
                return rc;
        }
        iommufd_ref_to_users(&new_ioas->obj);

-       if (access->ioas)
-               __iommufd_access_detach(access);
        access->ioas = new_ioas;
        access->ioas_unpin = new_ioas;
        return 0;
-------------------------------------------------------------------------------

Thanks
Nicolin