回复:[PATCH] ovl: fix mmap denywrite

[Chengguang Xu <cgxu519@mykernel.net>]

 ---- 在 星期二, 2021-06-22 20:30:04 Miklos Szeredi <miklos@szeredi.hu> 撰写 ----
 > Overlayfs did not honor positive i_writecount on realfile for VM_DENYWRITE
 > mappings.  Similarly negative i_mmap_writable counts were ignored for
 > VM_SHARED mappings.
 > 
 > Fix by making vma_set_file() switch the temporary counts obtained and
 > released by mmap_region().
 > 
 > Reported-by: Chengguang Xu <cgxu519@mykernel.net>
 > Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
 > ---
 >  fs/overlayfs/file.c |    4 +++-
 >  include/linux/mm.h  |    1 +
 >  mm/mmap.c           |    2 +-
 >  mm/util.c           |   38 +++++++++++++++++++++++++++++++++++++-
 >  4 files changed, 42 insertions(+), 3 deletions(-)
 > 
 > --- a/fs/overlayfs/file.c
 > +++ b/fs/overlayfs/file.c
 > @@ -430,7 +430,9 @@ static int ovl_mmap(struct file *file, s
 >      if (WARN_ON(file != vma->vm_file))
 >          return -EIO;
 >  
 > -    vma_set_file(vma, realfile);
 > +    ret = vma_set_file_checkwrite(vma, realfile);
 > +    if (ret)
 > +        return ret;

I'm afraid that it may affect other overlayfs instances which share lower layers(no upper),
so could we just check those permissions for upper layer?



 >  
 >      old_cred = ovl_override_creds(file_inode(file)->i_sb);
 >      ret = call_mmap(vma->vm_file, vma);
 > --- a/include/linux/mm.h
 > +++ b/include/linux/mm.h
 > @@ -2751,6 +2751,7 @@ static inline void vma_set_page_prot(str
 >  #endif
 >  
 >  void vma_set_file(struct vm_area_struct *vma, struct file *file);
 > +int vma_set_file_checkwrite(struct vm_area_struct *vma, struct file *file);
 >  
 >  #ifdef CONFIG_NUMA_BALANCING
 >  unsigned long change_prot_numa(struct vm_area_struct *vma,
 > --- a/mm/mmap.c
 > +++ b/mm/mmap.c
 > @@ -1809,6 +1809,7 @@ unsigned long mmap_region(struct file *f
 >           */
 >          vma->vm_file = get_file(file);
 >          error = call_mmap(file, vma);
 > +        file = vma->vm_file;


I'm not sure the behavior of changing vma_file is always safe for vma merging case.
In vma merging case, before go to tag 'unmap_writable' the reference of vma->vm_file will be released by fput().
For overlayfs, it probably safe because overlayfs file will get another reference for lower/upper file.



Thanks,
Chengguang Xu




 >          if (error)
 >              goto unmap_and_free_vma;
 >  
 > @@ -1870,7 +1871,6 @@ unsigned long mmap_region(struct file *f
 >          if (vm_flags & VM_DENYWRITE)
 >              allow_write_access(file);
 >      }
 > -    file = vma->vm_file;
 >  out:
 >      perf_event_mmap(vma);
 >  
 > --- a/mm/util.c
 > +++ b/mm/util.c
 > @@ -314,12 +314,48 @@ int vma_is_stack_for_current(struct vm_a
 >  /*
 >   * Change backing file, only valid to use during initial VMA setup.
 >   */
 > -void vma_set_file(struct vm_area_struct *vma, struct file *file)
 > +int vma_set_file_checkwrite(struct vm_area_struct *vma, struct file *file)
 >  {
 > +    vm_flags_t vm_flags = vma->vm_flags;
 > +    int err = 0;
 > +
 >      /* Changing an anonymous vma with this is illegal */
 >      get_file(file);
 > +
 > +    /* Get temporary denial counts on replacement */
 > +    if (vm_flags & VM_DENYWRITE) {
 > +        err = deny_write_access(file);
 > +        if (err)
 > +            goto out_put;
 > +    }
 > +    if (vm_flags & VM_SHARED) {
 > +        err = mapping_map_writable(file->f_mapping);
 > +        if (err)
 > +            goto out_allow;
 > +    }
 > +
 >      swap(vma->vm_file, file);
 > +
 > +    /* Undo temporary denial counts on replaced */
 > +    if (vm_flags & VM_SHARED)
 > +        mapping_unmap_writable(file->f_mapping);
 > +out_allow:
 > +    if (vm_flags & VM_DENYWRITE)
 > +        allow_write_access(file);
 > +out_put:
 >      fput(file);
 > +    return err;
 > +}
 > +EXPORT_SYMBOL(vma_set_file_checkwrite);
 > +
 > +/*
 > + * Change backing file, only valid to use during initial VMA setup.
 > + */
 > +void vma_set_file(struct vm_area_struct *vma, struct file *file)
 > +{
 > +    int err = vma_set_file_checkwrite(vma, file);
 > +
 > +    WARN_ON_ONCE(err);
 >  }
 >  EXPORT_SYMBOL(vma_set_file);
 >  
 > 
 > 
 >