* [PATCH] RDMA/siw: Fix missing permission check in user buffer registration
@ 2022-12-16 18:32 Bernard Metzler
2022-12-16 18:34 ` Jason Gunthorpe
0 siblings, 1 reply; 9+ messages in thread
From: Bernard Metzler @ 2022-12-16 18:32 UTC (permalink / raw)
To: linux-rdma; +Cc: jgg, leonro, David.Laight, Bernard Metzler
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(-)
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;
--
2.32.0
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] RDMA/siw: Fix missing permission check in user buffer registration
2022-12-16 18:32 [PATCH] RDMA/siw: Fix missing permission check in user buffer registration Bernard Metzler
@ 2022-12-16 18:34 ` Jason Gunthorpe
2022-12-16 20:11 ` Bernard Metzler
0 siblings, 1 reply; 9+ messages in thread
From: Jason Gunthorpe @ 2022-12-16 18:34 UTC (permalink / raw)
To: Bernard Metzler; +Cc: linux-rdma, leonro, David.Laight
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH] RDMA/siw: Fix missing permission check in user buffer registration
2022-12-16 18:34 ` Jason Gunthorpe
@ 2022-12-16 20:11 ` Bernard Metzler
2022-12-16 20:12 ` Jason Gunthorpe
0 siblings, 1 reply; 9+ messages in thread
From: Bernard Metzler @ 2022-12-16 20:11 UTC (permalink / raw)
To: Jason Gunthorpe; +Cc: linux-rdma, leonro, David.Laight
> -----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 😉
pin_user_pages_fast() -> internal_get_user_pages_fast() -> access_ok()
siw needs to call access_ok() during user buffer registration.
Which is done by pin_user_pages_fast(), but not by pin_user_pages()
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] RDMA/siw: Fix missing permission check in user buffer registration
2022-12-16 20:11 ` Bernard Metzler
@ 2022-12-16 20:12 ` Jason Gunthorpe
2022-12-20 13:52 ` Bernard Metzler
0 siblings, 1 reply; 9+ messages in thread
From: Jason Gunthorpe @ 2022-12-16 20:12 UTC (permalink / raw)
To: Bernard Metzler; +Cc: linux-rdma, leonro, David.Laight
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: Re: [PATCH] RDMA/siw: Fix missing permission check in user buffer registration
2022-12-16 20:12 ` Jason Gunthorpe
@ 2022-12-20 13:52 ` Bernard Metzler
2023-01-03 13:07 ` Jason Gunthorpe
0 siblings, 1 reply; 9+ messages in thread
From: Bernard Metzler @ 2022-12-20 13:52 UTC (permalink / raw)
To: Jason Gunthorpe; +Cc: linux-rdma, leonro, David.Laight
> -----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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Re: [PATCH] RDMA/siw: Fix missing permission check in user buffer registration
2022-12-20 13:52 ` Bernard Metzler
@ 2023-01-03 13:07 ` Jason Gunthorpe
2023-01-03 14:11 ` Bernard Metzler
0 siblings, 1 reply; 9+ messages in thread
From: Jason Gunthorpe @ 2023-01-03 13:07 UTC (permalink / raw)
To: Bernard Metzler; +Cc: linux-rdma, leonro, David.Laight
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: Re: [PATCH] RDMA/siw: Fix missing permission check in user buffer registration
2023-01-03 13:07 ` Jason Gunthorpe
@ 2023-01-03 14:11 ` Bernard Metzler
2023-01-03 14:15 ` Jason Gunthorpe
0 siblings, 1 reply; 9+ messages in thread
From: Bernard Metzler @ 2023-01-03 14:11 UTC (permalink / raw)
To: Jason Gunthorpe; +Cc: linux-rdma, leonro, David.Laight
> -----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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Re: [PATCH] RDMA/siw: Fix missing permission check in user buffer registration
2023-01-03 14:11 ` Bernard Metzler
@ 2023-01-03 14:15 ` Jason Gunthorpe
2023-01-03 16:19 ` Bernard Metzler
0 siblings, 1 reply; 9+ messages in thread
From: Jason Gunthorpe @ 2023-01-03 14:15 UTC (permalink / raw)
To: Bernard Metzler; +Cc: linux-rdma, leonro, David.Laight
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: Re: Re: [PATCH] RDMA/siw: Fix missing permission check in user buffer registration
2023-01-03 14:15 ` Jason Gunthorpe
@ 2023-01-03 16:19 ` Bernard Metzler
0 siblings, 0 replies; 9+ messages in thread
From: Bernard Metzler @ 2023-01-03 16:19 UTC (permalink / raw)
To: Jason Gunthorpe; +Cc: linux-rdma, leonro, David.Laight
> -----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.
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2023-01-03 16:19 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-12-16 18:32 [PATCH] RDMA/siw: Fix missing permission check in user buffer registration Bernard Metzler
2022-12-16 18:34 ` Jason Gunthorpe
2022-12-16 20:11 ` Bernard Metzler
2022-12-16 20:12 ` Jason Gunthorpe
2022-12-20 13:52 ` Bernard Metzler
2023-01-03 13:07 ` Jason Gunthorpe
2023-01-03 14:11 ` Bernard Metzler
2023-01-03 14:15 ` Jason Gunthorpe
2023-01-03 16:19 ` Bernard Metzler
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.