| Message ID | 20221216183209.21183-1-bmt@zurich.ibm.com (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
| Series | RDMA/siw: Fix missing permission check in user buffer registration | expand |
On Fri, Dec 16, 2022 at 07:32:09PM +0100, Bernard Metzler wrote: > User communication buffer registration lacks check of access > rights for provided address range. Using pin_user_pages_fast() > instead of pin_user_pages() during user page pinning implicitely > introduces the necessary check. It furthermore tries to avoid > grabbing the mmap_read_lock. Huh? What access check? Jason
> -----Original Message----- > From: Jason Gunthorpe <jgg@nvidia.com> > Sent: Friday, 16 December 2022 19:35 > To: Bernard Metzler <BMT@zurich.ibm.com> > Cc: linux-rdma@vger.kernel.org; leonro@nvidia.com; David.Laight@aculab.com > Subject: [EXTERNAL] Re: [PATCH] RDMA/siw: Fix missing permission check in > user buffer registration > > On Fri, Dec 16, 2022 at 07:32:09PM +0100, Bernard Metzler wrote: > > User communication buffer registration lacks check of access > > rights for provided address range. Using pin_user_pages_fast() > > instead of pin_user_pages() during user page pinning implicitely > > introduces the necessary check. It furthermore tries to avoid > > grabbing the mmap_read_lock. > > Huh? What access check? > if (unlikely(!access_ok((void __user *)start, len))) return -EFAULT; Seems I have to work on the commit message
On Fri, Dec 16, 2022 at 08:11:32PM +0000, Bernard Metzler wrote: > > > > -----Original Message----- > > From: Jason Gunthorpe <jgg@nvidia.com> > > Sent: Friday, 16 December 2022 19:35 > > To: Bernard Metzler <BMT@zurich.ibm.com> > > Cc: linux-rdma@vger.kernel.org; leonro@nvidia.com; David.Laight@aculab.com > > Subject: [EXTERNAL] Re: [PATCH] RDMA/siw: Fix missing permission check in > > user buffer registration > > > > On Fri, Dec 16, 2022 at 07:32:09PM +0100, Bernard Metzler wrote: > > > User communication buffer registration lacks check of access > > > rights for provided address range. Using pin_user_pages_fast() > > > instead of pin_user_pages() during user page pinning implicitely > > > introduces the necessary check. It furthermore tries to avoid > > > grabbing the mmap_read_lock. > > > > Huh? What access check? > > > > if (unlikely(!access_ok((void __user *)start, len))) > return -EFAULT; > > siw needs to call access_ok() during user buffer registration. No, it doesn't Either pin_user_pages or pin_user_pages_fast() are equivalent. You do have a bad bug here if this isn't holding the mmap lock though Jason
> -----Original Message----- > From: Jason Gunthorpe <jgg@nvidia.com> > Sent: Friday, 16 December 2022 21:13 > To: Bernard Metzler <BMT@zurich.ibm.com> > Cc: linux-rdma@vger.kernel.org; leonro@nvidia.com; > David.Laight@aculab.com > Subject: [EXTERNAL] Re: [PATCH] RDMA/siw: Fix missing permission check > in user buffer registration > > On Fri, Dec 16, 2022 at 08:11:32PM +0000, Bernard Metzler wrote: > > > > > > > -----Original Message----- > > > From: Jason Gunthorpe <jgg@nvidia.com> > > > Sent: Friday, 16 December 2022 19:35 > > > To: Bernard Metzler <BMT@zurich.ibm.com> > > > Cc: linux-rdma@vger.kernel.org; leonro@nvidia.com; > David.Laight@aculab.com > > > Subject: [EXTERNAL] Re: [PATCH] RDMA/siw: Fix missing permission > check in > > > user buffer registration > > > > > > On Fri, Dec 16, 2022 at 07:32:09PM +0100, Bernard Metzler wrote: > > > > User communication buffer registration lacks check of access > > > > rights for provided address range. Using pin_user_pages_fast() > > > > instead of pin_user_pages() during user page pinning implicitely > > > > introduces the necessary check. It furthermore tries to avoid > > > > grabbing the mmap_read_lock. > > > > > > Huh? What access check? > > > > > > > if (unlikely(!access_ok((void __user *)start, len))) > > return -EFAULT; > > > > siw needs to call access_ok() during user buffer registration. > > No, it doesn't > > Either pin_user_pages or pin_user_pages_fast() are equivalent. > > You do have a bad bug here if this isn't holding the mmap lock though > No, that lock is held. I was triggered by David's arguing about protection. I went down the path of pin_user_pages() and did not find a singe point where access rights to the buffer being registered are enforced. pin_user_pages_fast() do have it though. So I proposed a change in siw to use that function. Best, Bernard.
On Tue, Dec 20, 2022 at 01:52:43PM +0000, Bernard Metzler wrote: > > > > -----Original Message----- > > From: Jason Gunthorpe <jgg@nvidia.com> > > Sent: Friday, 16 December 2022 21:13 > > To: Bernard Metzler <BMT@zurich.ibm.com> > > Cc: linux-rdma@vger.kernel.org; leonro@nvidia.com; > > David.Laight@aculab.com > > Subject: [EXTERNAL] Re: [PATCH] RDMA/siw: Fix missing permission check > > in user buffer registration > > > > On Fri, Dec 16, 2022 at 08:11:32PM +0000, Bernard Metzler wrote: > > > > > > > > > > -----Original Message----- > > > > From: Jason Gunthorpe <jgg@nvidia.com> > > > > Sent: Friday, 16 December 2022 19:35 > > > > To: Bernard Metzler <BMT@zurich.ibm.com> > > > > Cc: linux-rdma@vger.kernel.org; leonro@nvidia.com; > > David.Laight@aculab.com > > > > Subject: [EXTERNAL] Re: [PATCH] RDMA/siw: Fix missing permission > > check in > > > > user buffer registration > > > > > > > > On Fri, Dec 16, 2022 at 07:32:09PM +0100, Bernard Metzler wrote: > > > > > User communication buffer registration lacks check of access > > > > > rights for provided address range. Using pin_user_pages_fast() > > > > > instead of pin_user_pages() during user page pinning implicitely > > > > > introduces the necessary check. It furthermore tries to avoid > > > > > grabbing the mmap_read_lock. > > > > > > > > Huh? What access check? > > > > > > > > > > if (unlikely(!access_ok((void __user *)start, len))) > > > return -EFAULT; > > > > > > siw needs to call access_ok() during user buffer registration. > > > > No, it doesn't > > > > Either pin_user_pages or pin_user_pages_fast() are equivalent. > > > > You do have a bad bug here if this isn't holding the mmap lock though > > > > No, that lock is held. I was triggered by David's arguing about > protection. I went down the path of pin_user_pages() and did > not find a singe point where access rights to the buffer being > registered are enforced. pin_user_pages_fast() do have it though. > So I proposed a change in siw to use that function. It checks it when it reads the PTEs Jason
> -----Original Message----- > From: Jason Gunthorpe <jgg@nvidia.com> > Sent: Tuesday, 3 January 2023 14:08 > To: Bernard Metzler <BMT@zurich.ibm.com> > Cc: linux-rdma@vger.kernel.org; leonro@nvidia.com; > David.Laight@aculab.com > Subject: [EXTERNAL] Re: Re: [PATCH] RDMA/siw: Fix missing permission > check in user buffer registration > > On Tue, Dec 20, 2022 at 01:52:43PM +0000, Bernard Metzler wrote: > > > > > > > -----Original Message----- > > > From: Jason Gunthorpe <jgg@nvidia.com> > > > Sent: Friday, 16 December 2022 21:13 > > > To: Bernard Metzler <BMT@zurich.ibm.com> > > > Cc: linux-rdma@vger.kernel.org; leonro@nvidia.com; > > > David.Laight@aculab.com > > > Subject: [EXTERNAL] Re: [PATCH] RDMA/siw: Fix missing permission > check > > > in user buffer registration > > > > > > On Fri, Dec 16, 2022 at 08:11:32PM +0000, Bernard Metzler wrote: > > > > > > > > > > > > > -----Original Message----- > > > > > From: Jason Gunthorpe <jgg@nvidia.com> > > > > > Sent: Friday, 16 December 2022 19:35 > > > > > To: Bernard Metzler <BMT@zurich.ibm.com> > > > > > Cc: linux-rdma@vger.kernel.org; leonro@nvidia.com; > > > David.Laight@aculab.com > > > > > Subject: [EXTERNAL] Re: [PATCH] RDMA/siw: Fix missing permission > > > check in > > > > > user buffer registration > > > > > > > > > > On Fri, Dec 16, 2022 at 07:32:09PM +0100, Bernard Metzler wrote: > > > > > > User communication buffer registration lacks check of access > > > > > > rights for provided address range. Using pin_user_pages_fast() > > > > > > instead of pin_user_pages() during user page pinning > implicitely > > > > > > introduces the necessary check. It furthermore tries to avoid > > > > > > grabbing the mmap_read_lock. > > > > > > > > > > Huh? What access check? > > > > > > > > > > > > > if (unlikely(!access_ok((void __user *)start, len))) > > > > return -EFAULT; > > > > > > > > siw needs to call access_ok() during user buffer registration. > > > > > > No, it doesn't > > > > > > Either pin_user_pages or pin_user_pages_fast() are equivalent. > > > > > > You do have a bad bug here if this isn't holding the mmap lock > though > > > > > > > No, that lock is held. I was triggered by David's arguing about > > protection. I went down the path of pin_user_pages() and did > > not find a singe point where access rights to the buffer being > > registered are enforced. pin_user_pages_fast() do have it though. > > So I proposed a change in siw to use that function. > > It checks it when it reads the PTEs > Oh ok, thanks. It is probably find_vma() further down the call which makes sure the address is valid for the user context. I tried reserving bogus user memory with siw and indeed get the right failure from trying to pin it. It may still make sense to use pin_user_pages_fast() instead? Taking the mmap_read_lock() explicitly within the driver could be avoided. Thank you! Bernard.
On Tue, Jan 03, 2023 at 02:11:19PM +0000, Bernard Metzler wrote: > Oh ok, thanks. It is probably find_vma() further down the call > which makes sure the address is valid for the user context. I > tried reserving bogus user memory with siw and indeed get the > right failure from trying to pin it. Why isn't this using a normal umem which does all this for you? Jason
> -----Original Message----- > From: Jason Gunthorpe <jgg@nvidia.com> > Sent: Tuesday, 3 January 2023 15:15 > To: Bernard Metzler <BMT@zurich.ibm.com> > Cc: linux-rdma@vger.kernel.org; leonro@nvidia.com; > David.Laight@aculab.com > Subject: [EXTERNAL] Re: Re: [PATCH] RDMA/siw: Fix missing permission > check in user buffer registration > > On Tue, Jan 03, 2023 at 02:11:19PM +0000, Bernard Metzler wrote: > > Oh ok, thanks. It is probably find_vma() further down the call > > which makes sure the address is valid for the user context. I > > tried reserving bogus user memory with siw and indeed get the > > right failure from trying to pin it. > > Why isn't this using a normal umem which does all this for you? > > Jason siw as a software only drivers sits on top of TCP sockets and just wants to read and write from/to pages. For efficiency in starting/resuming sending or receiving, it benefits from a simple mechanism to get the right page of a registered mem region as quick as possible. So it keeps the pages of a memory region in a two dimensional array, indexable with very simple arithmetic - see siw_get_upage(). ib_umem_get() provides a scatterlist of pages, which a sw provider would have to traverse linearly on the fast path to get to the right page to read/write. siw als does not care about dma addresses. I see other drivers, such as hfi or qib, which also do not use ib_umem_get(). siw could of course use ib_umem_get() first to get a scatterlist of pinned pages and translate this into an indexable internal presentation of pages, but I considered this an unacceptable waste of kernel resources. Bernard.
diff --git a/drivers/infiniband/sw/siw/siw_mem.c b/drivers/infiniband/sw/siw/siw_mem.c index b2b33dd3b4fa..ba28a5d94fc1 100644 --- a/drivers/infiniband/sw/siw/siw_mem.c +++ b/drivers/infiniband/sw/siw/siw_mem.c @@ -423,8 +423,8 @@ struct siw_umem *siw_umem_get(u64 start, u64 len, bool writable) while (nents) { struct page **plist = &umem->page_chunk[i].plist[got]; - rv = pin_user_pages(first_page_va, nents, foll_flags, - plist, NULL); + rv = pin_user_pages_fast(first_page_va, nents, + foll_flags, plist); if (rv < 0) goto out_sem_up;
User communication buffer registration lacks check of access rights for provided address range. Using pin_user_pages_fast() instead of pin_user_pages() during user page pinning implicitely introduces the necessary check. It furthermore tries to avoid grabbing the mmap_read_lock. Fixes: 2251334dcac9 ("rdma/siw: application buffer management") Signed-off-by: Bernard Metzler <bmt@zurich.ibm.com> --- drivers/infiniband/sw/siw/siw_mem.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)