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

[Pengfei Xu <pengfei.xu@intel.com>]

Hi Nicolin,

On 2023-06-23 at 15:10:35 -0700, Nicolin Chen wrote:
> 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.

Thanks a lot for your analysis and fixed patch!
I have installed your below patch on top of previous reproduced kernel.
And ran the reproduced binary in previous VM more than 1 hour, this issue
could not be reproduced, result show that it's fixed.
The bzImage with below patch is in link:
https://github.com/xupengfe/syzkaller_logs/blob/main/230614_070652_iopt_remove_access/bzImage_fix.gz

Thanks!
Best Regards,
Pengfei

> 
> 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