Re: [PATCH] ipc/shm: fix use-after-free of shm file via remap_file_pages()

[Eric Biggers]

On Mon, Apr 09, 2018 at 12:48:14PM +0300, Kirill A. Shutemov wrote:
> On Mon, Apr 09, 2018 at 04:30:39AM +0000, Eric Biggers wrote:
> > diff --git a/ipc/shm.c b/ipc/shm.c
> > index acefe44fefefa..c80c5691a9970 100644
> > --- a/ipc/shm.c
> > +++ b/ipc/shm.c
> > @@ -225,6 +225,12 @@ static int __shm_open(struct vm_area_struct *vma)
> >  	if (IS_ERR(shp))
> >  		return PTR_ERR(shp);
> >  
> > +	if (shp->shm_file != sfd->file) {
> > +		/* ID was reused */
> > +		shm_unlock(shp);
> > +		return -EINVAL;
> > +	}
> > +
> >  	shp->shm_atim = ktime_get_real_seconds();
> >  	ipc_update_pid(&shp->shm_lprid, task_tgid(current));
> >  	shp->shm_nattch++;
> > @@ -455,8 +461,9 @@ static int shm_mmap(struct file *file, struct vm_area_struct *vma)
> >  	int ret;
> >  
> >  	/*
> > -	 * In case of remap_file_pages() emulation, the file can represent
> > -	 * removed IPC ID: propogate shm_lock() error to caller.
> > +	 * In case of remap_file_pages() emulation, the file can represent an
> > +	 * IPC ID that was removed, and possibly even reused by another shm
> > +	 * segment already.  Propagate this case as an error to caller.
> >  	 */
> >  	ret = __shm_open(vma);
> >  	if (ret)
> > @@ -480,6 +487,7 @@ static int shm_release(struct inode *ino, struct file *file)
> >  	struct shm_file_data *sfd = shm_file_data(file);
> >  
> >  	put_ipc_ns(sfd->ns);
> > +	fput(sfd->file);
> >  	shm_file_data(file) = NULL;
> >  	kfree(sfd);
> >  	return 0;
> > @@ -1432,7 +1440,7 @@ long do_shmat(int shmid, char __user *shmaddr, int shmflg,
> >  	file->f_mapping = shp->shm_file->f_mapping;
> >  	sfd->id = shp->shm_perm.id;
> >  	sfd->ns = get_ipc_ns(ns);
> > -	sfd->file = shp->shm_file;
> > +	sfd->file = get_file(shp->shm_file);
> >  	sfd->vm_ops = NULL;
> >  
> >  	err = security_mmap_file(file, prot, flags);
> 
> Hm. Why do we need sfd->file refcounting now? It's not obvious to me.
> 
> Looks like it's either a separate bug or an unneeded change.
> 

It's necessary because if we don't hold a reference to sfd->file, then it can be
a stale pointer when we compare it in __shm_open().  In particular, if the new
struct file happened to be allocated at the same address as the old one, then
'sfd->file == shp->shm_file' so the mmap would be allowed.  But, it will be a
different shm segment than was intended.  The caller may not even have
permissions to map it normally, yet it would be done anyway.

In the end it's just broken to have a pointer to something that can be freed out
from under you...

- Eric