[PATCH] RDMA/siw: Fix missing permission check in user buffer registration

[PATCH] RDMA/siw: Fix missing permission check in user buffer registration

[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

Re: [PATCH] RDMA/siw: Fix missing permission check in user buffer registration

[Jason Gunthorpe]

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

RE: [PATCH] RDMA/siw: Fix missing permission check in user buffer registration

[Bernard Metzler]

> -----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()

Re: [PATCH] RDMA/siw: Fix missing permission check in user buffer registration

[Jason Gunthorpe]

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

RE: Re: [PATCH] RDMA/siw: Fix missing permission check in user buffer registration

[Bernard Metzler]

> -----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.

Re: Re: [PATCH] RDMA/siw: Fix missing permission check in user buffer registration

[Jason Gunthorpe]

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

RE: Re: [PATCH] RDMA/siw: Fix missing permission check in user buffer registration

[Bernard Metzler]

> -----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.

Re: Re: [PATCH] RDMA/siw: Fix missing permission check in user buffer registration

[Jason Gunthorpe]

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

RE: Re: Re: [PATCH] RDMA/siw: Fix missing permission check in user buffer registration

[Bernard Metzler]

> -----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.