On Mon, Jan 07, 2019 at 04:43:20PM +0200, Gal Pressman wrote: > On 06-Jan-19 23:30, Jason Gunthorpe wrote: > > On Sun, Jan 06, 2019 at 03:33:15PM +0200, Gal Pressman wrote: > > > >>> Otherwise there can be use-after free style security bugs. > >>> > >>> Since the efa_dealloc_ucontext does nothing, and BAR pages are being > >>> mapped, it must be wrong. > >> > >> Can you please elaborate? what would you like to see in dealloc_ucontext? > > > > Freeing bar page allocations. > > > >>> It kind of looks like it is trying to tie BAR allocation lifetime to > >>> individual objects? > >>> > >>> .. and all of this is why one generally focuses on the ucontext as the > >>> limit, as generally, allocating a ucontext implies allocating a BAR > >>> page, and thus the number of ucontexts is strictly limited by the BAR > >>> size. > >> > >> s/ucontext/PD/g is the case for EFA, our device is not aware of > >> ucontext but PDs. The BAR "reservation" is there for the lifetime of > >> the PD. > > > > Which is what I just said was wrong. > > It's different, I still can't see how it's wrong. > Our device is responsible for the BAR reservations, not the driver. > The device ties it to the lifetime of the object, as long as the object lives > the mapping is valid. No, so long as the *mapping* is accessible to userspace the BAR pages must be reserved and not re-used for any other purpose. Otherwise you get in a situation where two processes have mmaps to the same BAR pages when they really shouldn't, and you are back to proving that your HW is secure to operate in that mode. The lifetime of the mapping has nothing to do with the lifetime of the object, you cannot allow object destruction to recyle the BAR page to another object allocation. The device *cannot* control the bar page assignement because it has no information about the lifetime of the mapping of pages to userspace. The general pattern is the BAR pages are linked to the uncontext, because only destruction of the ucontext confirms that all userspace visibility of the pages has been completely removed. Jason
