Re: [PATCH -next 1/2] mm/memfd: make F_SEAL_FUTURE_WRITE seal more robust

[Joel Fernandes]

On Wed, Nov 21, 2018 at 07:25:26PM -0800, Andy Lutomirski wrote:
> On Wed, Nov 21, 2018 at 6:27 PM Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> wrote:
> >
> > On Tue, 20 Nov 2018 13:13:35 -0800 Joel Fernandes <joel@xxxxxxxxxxxxxxxxx> wrote:
> >
> > > > > I am Ok with whatever Andrew wants to do, if it is better to squash it with
> > > > > the original, then I can do that and send another patch.
> > > > >
> > > > >
> > > >
> > > > From experience, Andrew will food in fixups on request :)
> > >
> > > Andrew, could you squash this patch into the one titled ("mm: Add an
> > > F_SEAL_FUTURE_WRITE seal to memfd")?
> >
> > Sure.
> >
> > I could of course queue them separately but I rarely do so - I don't
> > think that the intermediate development states are useful in the
> > infinite-term, and I make them available via additional Link: tags in
> > the changelog footers anyway.
> >
> > I think that the magnitude of these patches is such that John Stultz's
> > Reviewed-by is invalidated, so this series is now in the "unreviewed"
> > state.
> >
> > So can we have a re-review please?  For convenience, here's the
> > folded-together [1/1] patch, as it will go to Linus.

Sure, I removed the old tags and also provide an updated patch below inline.

> > From: "Joel Fernandes (Google)" <joel@xxxxxxxxxxxxxxxxx>
> > Subject: mm: Add an F_SEAL_FUTURE_WRITE seal to memfd
> >
> > Android uses ashmem for sharing memory regions.  We are looking forward to
> > migrating all usecases of ashmem to memfd so that we can possibly remove
> > the ashmem driver in the future from staging while also benefiting from
> > using memfd and contributing to it.  Note staging drivers are also not ABI
> > and generally can be removed at anytime.
[...]
> > --- a/include/uapi/linux/fcntl.h~mm-add-an-f_seal_future_write-seal-to-memfd
> > +++ a/include/uapi/linux/fcntl.h
> > @@ -41,6 +41,7 @@
> >  #define F_SEAL_SHRINK  0x0002  /* prevent file from shrinking */
> >  #define F_SEAL_GROW    0x0004  /* prevent file from growing */
> >  #define F_SEAL_WRITE   0x0008  /* prevent writes */
> > +#define F_SEAL_FUTURE_WRITE    0x0010  /* prevent future writes while mapped */
> >  /* (1U << 31) is reserved for signed error codes */
> >
> >  /*
> > --- a/mm/memfd.c~mm-add-an-f_seal_future_write-seal-to-memfd
> > +++ a/mm/memfd.c
> > @@ -131,7 +131,8 @@ static unsigned int *memfd_file_seals_pt
> >  #define F_ALL_SEALS (F_SEAL_SEAL | \
> >                      F_SEAL_SHRINK | \
> >                      F_SEAL_GROW | \
> > -                    F_SEAL_WRITE)
> > +                    F_SEAL_WRITE | \
> > +                    F_SEAL_FUTURE_WRITE)
> >
> >  static int memfd_add_seals(struct file *file, unsigned int seals)
> >  {
> > --- a/fs/hugetlbfs/inode.c~mm-add-an-f_seal_future_write-seal-to-memfd
> > +++ a/fs/hugetlbfs/inode.c
> > @@ -530,7 +530,7 @@ static long hugetlbfs_punch_hole(struct
> >                 inode_lock(inode);
> >
> >                 /* protected by i_mutex */
> > -               if (info->seals & F_SEAL_WRITE) {
> > +               if (info->seals & (F_SEAL_WRITE | F_SEAL_FUTURE_WRITE)) {
> >                         inode_unlock(inode);
> >                         return -EPERM;
> >                 }
> > --- a/mm/shmem.c~mm-add-an-f_seal_future_write-seal-to-memfd
> > +++ a/mm/shmem.c
> > @@ -2119,6 +2119,23 @@ out_nomem:
> >
> >  static int shmem_mmap(struct file *file, struct vm_area_struct *vma)
> >  {
> > +       struct shmem_inode_info *info = SHMEM_I(file_inode(file));
> > +
> > +       /*
> > +        * New PROT_READ and MAP_SHARED mmaps are not allowed when "future
> 
> PROT_WRITE, perhaps?

Yes, fixed.

> > +        * write" seal active.
> > +        */
> > +       if ((vma->vm_flags & VM_SHARED) && (vma->vm_flags & VM_WRITE) &&
> > +           (info->seals & F_SEAL_FUTURE_WRITE))
> > +               return -EPERM;
> > +
> > +       /*
> > +        * Since the F_SEAL_FUTURE_WRITE seals allow for a MAP_SHARED read-only
> > +        * mapping, take care to not allow mprotect to revert protections.
> > +        */
> > +       if (info->seals & F_SEAL_FUTURE_WRITE)
> > +               vma->vm_flags &= ~(VM_MAYWRITE);
> > +
> 
> This might all be clearer as:
> 
> if (info->seals & F_SEAL_FUTURE_WRITE) {
>   if (vma->vm_flags ...)
>     return -EPERM;
>   vma->vm_flags &= ~VM_MAYWRITE;
> }
> 
> with appropriate comments inserted.

Agreed, its simpler. Updated patch is below. I squashed it with all the
earlier ones. Andy, could you provide Acks and/or Reviewed-by tag as well?

---8<-----------------------