summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorHugh Dickins <hugh@veritas.com>2006-04-12 14:34:27 -0700
committerGreg Kroah-Hartman <gregkh@suse.de>2006-04-17 14:24:58 -0700
commitb78b6af66a5fbaf17d7e6bfc32384df5e34408c8 (patch)
treebc5baa898697fcb6b1c0e8c1311ad4091ff98326
parentca1e0484d9fe8a9048ac32b0f9894545f43704e8 (diff)
downloadop-kernel-dev-b78b6af66a5fbaf17d7e6bfc32384df5e34408c8.zip
op-kernel-dev-b78b6af66a5fbaf17d7e6bfc32384df5e34408c8.tar.gz
[PATCH] shmat: stop mprotect from giving write permission to a readonly attachment (CVE-2006-1524)
I found that all of 2.4 and 2.6 have been letting mprotect give write permission to a readonly attachment of shared memory, whether or not IPC would give the caller that permission. SUS says "The behaviour of this function [mprotect] is unspecified if the mapping was not established by a call to mmap", but I don't think we can interpret that as allowing it to subvert IPC permissions. I haven't tried 2.2, but the 2.2.26 source looks like it gets it right; and the patch below reproduces that behaviour - mprotect cannot be used to add write permission to a shared memory segment attached readonly. This patch is simple, and I'm sure it's what we should have done in 2.4.0: if you want to go on to switch write permission on and off with mprotect, just don't attach the segment readonly in the first place. However, we could have accumulated apps which attach readonly (even though they would be permitted to attach read/write), and which subsequently use mprotect to switch write permission on and off: it's not unreasonable. I was going to add a second ipcperms check in do_shmat, to check for writable when readonly, and if not writable find_vma and clear VM_MAYWRITE. But security_ipc_permission might do auditing, and it seems wrong to report an attempt for write permission when there has been none. Or we could flag the vma as SHM, note the shmid or shp in vm_private_data, and then get mprotect to check. But the patch below is a lot simpler: I'd rather stick with it, if we can convince ourselves somehow that it'll be safe. Signed-off-by: Hugh Dickins <hugh@veritas.com> Signed-off-by: Andrew Morton <akpm@osdl.org> Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>
-rw-r--r--ipc/shm.c2
1 files changed, 2 insertions, 0 deletions
diff --git a/ipc/shm.c b/ipc/shm.c
index 6b0c9af..1c2faf6 100644
--- a/ipc/shm.c
+++ b/ipc/shm.c
@@ -162,6 +162,8 @@ static int shm_mmap(struct file * file, struct vm_area_struct * vma)
ret = shmem_mmap(file, vma);
if (ret == 0) {
vma->vm_ops = &shm_vm_ops;
+ if (!(vma->vm_flags & VM_WRITE))
+ vma->vm_flags &= ~VM_MAYWRITE;
shm_inc(file->f_dentry->d_inode->i_ino);
}
OpenPOWER on IntegriCloud