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

[Jason Gunthorpe <jgg@nvidia.com>]

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