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

[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.

> 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.