On Wed, Jan 20, 2021 at 08:35:04PM +0000, Matthew Wilcox wrote:
> On Wed, Jan 20, 2021 at 08:06:07PM +0200, Mike Rapoport wrote:
> > +static struct page *secretmem_alloc_page(gfp_t gfp)
> > +{
> > + /*
> > + * FIXME: use a cache of large pages to reduce the direct map
> > + * fragmentation
> > + */
> > + return alloc_page(gfp);
> > +}
> > +
> > +static vm_fault_t secretmem_fault(struct vm_fault *vmf)
> > +{
> > + struct address_space *mapping = vmf->vma->vm_file->f_mapping;
> > + struct inode *inode = file_inode(vmf->vma->vm_file);
> > + pgoff_t offset = vmf->pgoff;
> > + unsigned long addr;
> > + struct page *page;
> > + int err;
> > +
> > + if (((loff_t)vmf->pgoff << PAGE_SHIFT) >= i_size_read(inode))
> > + return vmf_error(-EINVAL);
> > +
> > +retry:
> > + page = find_lock_page(mapping, offset);
> > + if (!page) {
> > + page = secretmem_alloc_page(vmf->gfp_mask);
> > + if (!page)
> > + return VM_FAULT_OOM;
> > +
> > + err = set_direct_map_invalid_noflush(page, 1);
> > + if (err)
> > + return vmf_error(err);
>
> Haven't we leaked the page at this point?
Well, yes. :(
But this code is anyway changed in the next patch. Is this really so
important to fix this in the middle of the series?
> > + __SetPageUptodate(page);
> > + err = add_to_page_cache(page, mapping, offset, vmf->gfp_mask);
>
> At this point, doesn't the page contain data from the last person to use
> the page? ie we've leaked data to this process? I don't see anywhere
> that we write data to the page.
The data is visible for all processes that share the file descriptor. So
no, we don't leak anything unless the file descriptor itself is leaked.
Did you have a particular scenario in mind?
--
Sincerely yours,
Mike.
