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

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

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
>