Re: [RFC v4][PATCH 5/9] Memory managemnet (restore)

["Serge E. Hallyn" <serue@us.ibm.com>]

Quoting Oren Laadan (orenl@cs.columbia.edu):
> 
> 
> Serge E. Hallyn wrote:
> > Quoting Oren Laadan (orenl@cs.columbia.edu):
> 
> [...]
> 
> >> +/* change the protection of an address range to be writable/non-writable.
> >> + * this is useful when restoring the memory of a read-only vma */
> >> +static int cr_vma_set_writable(struct mm_struct *mm, unsigned long start,
> >> +			       unsigned long end, int writable)
> >> +{
> >> +	struct vm_area_struct *vma, *prev;
> >> +	unsigned long flags = 0;
> >> +	int ret = -EINVAL;
> >> +
> >> +	cr_debug("vma %#lx-%#lx writable %d\n", start, end, writable);
> >> +
> >> +	down_write(&mm->mmap_sem);
> >> +	vma = find_vma_prev(mm, start, &prev);
> >> +	if (!vma || vma->vm_start > end || vma->vm_end < start)
> >> +		goto out;
> >> +	if (writable && !(vma->vm_flags & VM_WRITE))
> >> +		flags = vma->vm_flags | VM_WRITE;
> >> +	else if (!writable && (vma->vm_flags & VM_WRITE))
> >> +		flags = vma->vm_flags & ~VM_WRITE;
> >> +	cr_debug("flags %#lx\n", flags);
> >> +	if (flags)
> >> +		ret = mprotect_fixup(vma, &prev, vma->vm_start,
> >> +				     vma->vm_end, flags);
> > 
> > As Dave has pointed out, this appears to be a security problem.  I think
> 
> As I replied to Dave, I don't see why this would be a security problem.
> 
> This handles private memory only. In particular, the uncommon case of a
> read-only VMA tha has modified contents. This _cannot_ affect the file
> from which this VMA may have been mapped.
> 
> Shared memory (not file-mapped) will be handled differently: since it is
> always backed up by an inode in shmfs, the restart will populate the
> relevant pages directly. Besides, non-file-mapped shared memory is again
> not a security concern.
> 
> Finally, shared memory that maps to a file is simply _not saved_ at all;
> it is part of the file system, and belongs to the (future) file system
> snapshot capability. Since the contents are always available in the file
> system, we don't need to save it (like we don't save shared libraries).
> 
> That said, it is necessary that the code ensures that the vm_flags that
> belong to a VMA of a private type, e.g. CR_VMA_ANON/CR_VMA_FILE, indeed
> match it (ie, don't have VM_MAY_SHARE/VM_SHARED). I'll add that.

Cool.  That sounds good and I'll look for that in the next version.

There still may be objections about bypassing selinux execmem/execheap
permission checks, but I think that's ok for now.  Long-term I expect
we'll want the security_file_mprotect checks there, and selinux users
will have to use a policy where restart is started in a privileged
restart_t domain or somesuch (and eventually transitions back to the
checkpointed selinux type if possible).

thanks,
-serge

> > what you need to do is create a new helper mprotect_fixup_withchecks(),
> > which does all the DAC+MAC checks which are done in the sys_mprotect()
> > loop starting with "for (nstart = start ; ; ) {...  Otherwise an
> > unprivileged user can create a checkpoint image of a program which has
> > done a ro shared file mmap, edit the checkpoint, then restart it and (i
> > assume) cause the modified contents to be written to the file.  This
> > could violate both DAC checks and selinux checks.
> > 
> > So create that helper which does the security checks, and use it
> > both here and in the sys_mprotect() loop, please.
> > 
> 
> [...]
> 
> Oren.