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

From: Serge E. Hallyn
Date: Wed Sep 10 2008 - 11:00:45 EST


Quoting Oren Laadan (orenl@xxxxxxxxxxxxxxx):
>
>
> Serge E. Hallyn wrote:
> > Quoting Oren Laadan (orenl@xxxxxxxxxxxxxxx):
>
> [...]
>
> >> +/* 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/