> -----Original Message----- > From: Jason Gunthorpe <jgg@xxxxxxxxxx> > Sent: Tuesday, 3 January 2023 14:08 > To: Bernard Metzler <BMT@xxxxxxxxxxxxxx> > Cc: linux-rdma@xxxxxxxxxxxxxxx; leonro@xxxxxxxxxx; > David.Laight@xxxxxxxxxx > 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@xxxxxxxxxx> > > > Sent: Friday, 16 December 2022 21:13 > > > To: Bernard Metzler <BMT@xxxxxxxxxxxxxx> > > > Cc: linux-rdma@xxxxxxxxxxxxxxx; leonro@xxxxxxxxxx; > > > David.Laight@xxxxxxxxxx > > > 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@xxxxxxxxxx> > > > > > Sent: Friday, 16 December 2022 19:35 > > > > > To: Bernard Metzler <BMT@xxxxxxxxxxxxxx> > > > > > Cc: linux-rdma@xxxxxxxxxxxxxxx; leonro@xxxxxxxxxx; > > > David.Laight@xxxxxxxxxx > > > > > 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.
