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

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

[Pengfei Xu]

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!

---

If you don't need the following environment to reproduce the problem or if you
already have one, please ignore the following information.

How to reproduce:
git clone https://gitlab.com/xupengfe/repro_vm_env.git
cd repro_vm_env
tar -xvf repro_vm_env.tar.gz
cd repro_vm_env; ./start3.sh  // it needs qemu-system-x86_64 and I used v7.1.0
  // start3.sh will load bzImage_2241ab53cbb5cdb08a6b2d4688feb13971058f65 v6.2-rc5 kernel
  // You could change the bzImage_xxx as you want
  // Maybe you need to remove line "-drive if=pflash,format=raw,readonly=on,file=./OVMF_CODE.fd \" for different qemu version
You could use below command to log in, there is no password for root.
ssh -p 10023 root@localhost

After login vm(virtual machine) successfully, you could transfer reproduced
binary to the vm by below way, and reproduce the problem in vm:
gcc -pthread -o repro repro.c
scp -P 10023 repro root@localhost:/root/

Get the bzImage for target kernel:
Please use target kconfig and copy it to kernel_src/.config
make olddefconfig
make -jx bzImage           //x should equal or less than cpu num your pc has

Fill the bzImage file into above start3.sh to load the target kernel in vm.


Tips:
If you already have qemu-system-x86_64, please ignore below info.
If you want to install qemu v7.1.0 version:
git clone https://github.com/qemu/qemu.git
cd qemu
git checkout -f v7.1.0
mkdir build
cd build
yum install -y ninja-build.x86_64
yum -y install libslirp-devel.x86_64
../configure --target-list=x86_64-softmmu --enable-kvm --enable-vnc --enable-gtk --enable-sdl --enable-usb-redir --enable-slirp
make
make install

Best Regards,
Thanks!

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]

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

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]

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

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]

On Sun, Jun 25, 2023 at 05:30:32PM +0800, Pengfei Xu wrote:

> > 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 for retesting!

I've squashed this into the new version of my replace() series.

Nicolin