diff mbox series

[2/4] iommufd: Fix unpinning of pages when an access is present

Message ID 2-v1-ceab6a4d7d7a+94-iommufd_syz_jgg@nvidia.com (mailing list archive)
State Accepted
Commit 727c28c1cef2bc013d2c8bb6c50e410a3882a04e
Headers show
Series Fix three syzkaller splats in iommufd | expand

Commit Message

Jason Gunthorpe March 31, 2023, 3:32 p.m. UTC 
syzkaller found that the calculation of batch_last_index should use
'start_index' since at input to this function the batch is either empty or
it has already been adjusted to cross any accesses so it will start at the
point we are unmapping from.

Getting this wrong causes the unmap to run over the end of the pages
which corrupts pages that were never mapped. In most cases this triggers
the num pinned debugging:

  WARNING: CPU: 0 PID: 557 at drivers/iommu/iommufd/pages.c:294 __iopt_area_unfill_domain+0x152/0x560
  Modules linked in:
  CPU: 0 PID: 557 Comm: repro Not tainted 6.3.0-rc2-eeac8ede1755 #1
  Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.16.0-0-gd239552ce722-prebuilt.qemu.org 04/01/2014
  RIP: 0010:__iopt_area_unfill_domain+0x152/0x560
  Code: d2 0f ff 44 8b 64 24 54 48 8b 44 24 48 31 ff 44 89 e6 48 89 44 24 38 e8 fc d3 0f ff 45 85 e4 0f 85 eb 01 00 00 e8 0e d2 0f ff <0f> 0b e8 07 d2 0f ff 48 8b 44 24 38 89 5c 24 58 89 18 8b 44 24 54
  RSP: 0018:ffffc9000108baf0 EFLAGS: 00010246
  RAX: 0000000000000000 RBX: 00000000ffffffff RCX: ffffffff821e3f85
  RDX: 0000000000000000 RSI: ffff88800faf0000 RDI: 0000000000000002
  RBP: ffffc9000108bd18 R08: 000000000003ca25 R09: 0000000000000014
  R10: 000000000003ca00 R11: 0000000000000024 R12: 0000000000000004
  R13: 0000000000000801 R14: 00000000000007ff R15: 0000000000000800
  FS:  00007f3499ce1740(0000) GS:ffff88807dc00000(0000) knlGS:0000000000000000
  CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
  CR2: 0000000020000243 CR3: 00000000179c2001 CR4: 0000000000770ef0
  PKRU: 55555554
  Call Trace:
   <TASK>
   iopt_area_unfill_domain+0x32/0x40
   iopt_table_remove_domain+0x23f/0x4c0
   iommufd_device_selftest_detach+0x3a/0x90
   iommufd_selftest_destroy+0x55/0x70
   iommufd_object_destroy_user+0xce/0x130
   iommufd_destroy+0xa2/0xc0
   iommufd_fops_ioctl+0x206/0x330
   __x64_sys_ioctl+0x10e/0x160
   do_syscall_64+0x3b/0x90
   entry_SYSCALL_64_after_hwframe+0x72/0xdc

Also add some useful WARN_ON sanity checks.

Cc: <stable@vger.kernel.org>
Fixes: 8d160cd4d506 ("iommufd: Algorithms for PFN storage")
Reported-by: Pengfei Xu <pengfei.xu@intel.com>
Link: https://lore.kernel.org/r/ZBE1k040xAhIuTmq@xpf.sh.intel.com
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/iommufd/pages.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

Comments

Pengfei Xu April 1, 2023, 6:23 a.m. UTC | #1 
Hi Jason,

On 2023-03-31 at 12:32:25 -0300, Jason Gunthorpe wrote:
> syzkaller found that the calculation of batch_last_index should use
> 'start_index' since at input to this function the batch is either empty or
> it has already been adjusted to cross any accesses so it will start at the
> point we are unmapping from.
> 
> Getting this wrong causes the unmap to run over the end of the pages
> which corrupts pages that were never mapped. In most cases this triggers
> the num pinned debugging:
> 
>   WARNING: CPU: 0 PID: 557 at drivers/iommu/iommufd/pages.c:294 __iopt_area_unfill_domain+0x152/0x560
>   Modules linked in:
>   CPU: 0 PID: 557 Comm: repro Not tainted 6.3.0-rc2-eeac8ede1755 #1
>   Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.16.0-0-gd239552ce722-prebuilt.qemu.org 04/01/2014
>   RIP: 0010:__iopt_area_unfill_domain+0x152/0x560
>   Code: d2 0f ff 44 8b 64 24 54 48 8b 44 24 48 31 ff 44 89 e6 48 89 44 24 38 e8 fc d3 0f ff 45 85 e4 0f 85 eb 01 00 00 e8 0e d2 0f ff <0f> 0b e8 07 d2 0f ff 48 8b 44 24 38 89 5c 24 58 89 18 8b 44 24 54
>   RSP: 0018:ffffc9000108baf0 EFLAGS: 00010246
>   RAX: 0000000000000000 RBX: 00000000ffffffff RCX: ffffffff821e3f85
>   RDX: 0000000000000000 RSI: ffff88800faf0000 RDI: 0000000000000002
>   RBP: ffffc9000108bd18 R08: 000000000003ca25 R09: 0000000000000014
>   R10: 000000000003ca00 R11: 0000000000000024 R12: 0000000000000004
>   R13: 0000000000000801 R14: 00000000000007ff R15: 0000000000000800
>   FS:  00007f3499ce1740(0000) GS:ffff88807dc00000(0000) knlGS:0000000000000000
>   CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>   CR2: 0000000020000243 CR3: 00000000179c2001 CR4: 0000000000770ef0
>   PKRU: 55555554
>   Call Trace:
>    <TASK>
>    iopt_area_unfill_domain+0x32/0x40
>    iopt_table_remove_domain+0x23f/0x4c0
>    iommufd_device_selftest_detach+0x3a/0x90
>    iommufd_selftest_destroy+0x55/0x70
>    iommufd_object_destroy_user+0xce/0x130
>    iommufd_destroy+0xa2/0xc0
>    iommufd_fops_ioctl+0x206/0x330
>    __x64_sys_ioctl+0x10e/0x160
>    do_syscall_64+0x3b/0x90
>    entry_SYSCALL_64_after_hwframe+0x72/0xdc
> 
> Also add some useful WARN_ON sanity checks.
> 
> Cc: <stable@vger.kernel.org>
> Fixes: 8d160cd4d506 ("iommufd: Algorithms for PFN storage")
> Reported-by: Pengfei Xu <pengfei.xu@intel.com>
> Link: https://lore.kernel.org/r/ZBE1k040xAhIuTmq@xpf.sh.intel.com
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>  drivers/iommu/iommufd/pages.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/iommufd/pages.c b/drivers/iommu/iommufd/pages.c
> index 400ec7c91ed7e7..b11aace836542d 100644
> --- a/drivers/iommu/iommufd/pages.c
> +++ b/drivers/iommu/iommufd/pages.c
> @@ -1207,13 +1207,21 @@ iopt_area_unpin_domain(struct pfn_batch *batch, struct iopt_area *area,
>  			unsigned long start =
>  				max(start_index, *unmapped_end_index);
>  
> +			if (IS_ENABLED(CONFIG_IOMMUFD_TEST) &&
> +			    batch->total_pfns)
> +				WARN_ON(*unmapped_end_index -
> +						batch->total_pfns !=
> +					start_index);
>  			batch_from_domain(batch, domain, area, start,
>  					  last_index);
> -			batch_last_index = start + batch->total_pfns - 1;
> +			batch_last_index = start_index + batch->total_pfns - 1;
>  		} else {
>  			batch_last_index = last_index;
>  		}
>  
> +		if (IS_ENABLED(CONFIG_IOMMUFD_TEST))
> +			WARN_ON(batch_last_index > real_last_index);
> +
>  		/*
>  		 * unmaps must always 'cut' at a place where the pfns are not
>  		 * contiguous to pair with the maps that always install
> -- 
  I tested the reproduced code in the kernel with all 3 fixed patches.
  Syzkaller reproduced code: https://github.com/xupengfe/syzkaller_logs/raw/main/230314_094459___iopt_area_unfill_domain/repro
  This issue was gone and the issue was fixed.

  Thanks!
  BR.
> 2.40.0
>
Tian, Kevin April 4, 2023, 9:43 a.m. UTC | #2 
> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Friday, March 31, 2023 11:32 PM
> 
> syzkaller found that the calculation of batch_last_index should use
> 'start_index' since at input to this function the batch is either empty or
> it has already been adjusted to cross any accesses so it will start at the
> point we are unmapping from.
> 
> Getting this wrong causes the unmap to run over the end of the pages
> which corrupts pages that were never mapped. In most cases this triggers
> the num pinned debugging:
> 
>   WARNING: CPU: 0 PID: 557 at drivers/iommu/iommufd/pages.c:294
> __iopt_area_unfill_domain+0x152/0x560
>   Modules linked in:
>   CPU: 0 PID: 557 Comm: repro Not tainted 6.3.0-rc2-eeac8ede1755 #1
>   Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.16.0-0-
> gd239552ce722-prebuilt.qemu.org 04/01/2014
>   RIP: 0010:__iopt_area_unfill_domain+0x152/0x560
>   Code: d2 0f ff 44 8b 64 24 54 48 8b 44 24 48 31 ff 44 89 e6 48 89 44 24 38 e8
> fc d3 0f ff 45 85 e4 0f 85 eb 01 00 00 e8 0e d2 0f ff <0f> 0b e8 07 d2 0f ff 48 8b
> 44 24 38 89 5c 24 58 89 18 8b 44 24 54
>   RSP: 0018:ffffc9000108baf0 EFLAGS: 00010246
>   RAX: 0000000000000000 RBX: 00000000ffffffff RCX: ffffffff821e3f85
>   RDX: 0000000000000000 RSI: ffff88800faf0000 RDI: 0000000000000002
>   RBP: ffffc9000108bd18 R08: 000000000003ca25 R09: 0000000000000014
>   R10: 000000000003ca00 R11: 0000000000000024 R12: 0000000000000004
>   R13: 0000000000000801 R14: 00000000000007ff R15: 0000000000000800
>   FS:  00007f3499ce1740(0000) GS:ffff88807dc00000(0000)
> knlGS:0000000000000000
>   CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>   CR2: 0000000020000243 CR3: 00000000179c2001 CR4: 0000000000770ef0
>   PKRU: 55555554
>   Call Trace:
>    <TASK>
>    iopt_area_unfill_domain+0x32/0x40
>    iopt_table_remove_domain+0x23f/0x4c0
>    iommufd_device_selftest_detach+0x3a/0x90
>    iommufd_selftest_destroy+0x55/0x70
>    iommufd_object_destroy_user+0xce/0x130
>    iommufd_destroy+0xa2/0xc0
>    iommufd_fops_ioctl+0x206/0x330
>    __x64_sys_ioctl+0x10e/0x160
>    do_syscall_64+0x3b/0x90
>    entry_SYSCALL_64_after_hwframe+0x72/0xdc
> 
> Also add some useful WARN_ON sanity checks.
> 
> Cc: <stable@vger.kernel.org>
> Fixes: 8d160cd4d506 ("iommufd: Algorithms for PFN storage")
> Reported-by: Pengfei Xu <pengfei.xu@intel.com>
> Link: https://lore.kernel.org/r/ZBE1k040xAhIuTmq@xpf.sh.intel.com
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>
diff mbox series

Patch

diff --git a/drivers/iommu/iommufd/pages.c b/drivers/iommu/iommufd/pages.c
index 400ec7c91ed7e7..b11aace836542d 100644
--- a/drivers/iommu/iommufd/pages.c
+++ b/drivers/iommu/iommufd/pages.c
@@ -1207,13 +1207,21 @@  iopt_area_unpin_domain(struct pfn_batch *batch, struct iopt_area *area,
 			unsigned long start =
 				max(start_index, *unmapped_end_index);
 
+			if (IS_ENABLED(CONFIG_IOMMUFD_TEST) &&
+			    batch->total_pfns)
+				WARN_ON(*unmapped_end_index -
+						batch->total_pfns !=
+					start_index);
 			batch_from_domain(batch, domain, area, start,
 					  last_index);
-			batch_last_index = start + batch->total_pfns - 1;
+			batch_last_index = start_index + batch->total_pfns - 1;
 		} else {
 			batch_last_index = last_index;
 		}
 
+		if (IS_ENABLED(CONFIG_IOMMUFD_TEST))
+			WARN_ON(batch_last_index > real_last_index);
+
 		/*
 		 * unmaps must always 'cut' at a place where the pfns are not
 		 * contiguous to pair with the maps that always install