[PATCH] hugetlbfs private mappings.

[PATCH] hugetlbfs private mappings.

[Oleg Nesterov]

Hello.

Hugetlbfs silently coerce private mappings of hugetlb files
into shared ones. So private writable mapping has MAP_SHARED
semantics. I think, such mappings should be disallowed.

First, such behavior allows open hugetlbfs file O_RDONLY, and
overwrite it via mmap(PROT_READ|PROT_WRITE, MAP_PRIVATE), so
it is security bug.

Second, private writable mmap() should fail just because kernel
does not support this.

I beleive, it is ok to allow private readonly hugetlb mappings,
sys_mprotect() does not work with hugetlb vmas.

There is another problem. Hugetlb mapping is always prefaulted,
pages allocated at mmap() time. So even readonly mapping allows
to enlarge the size of the hugetlbfs file, and steal huge pages
without appropriative permissions.

Patch on top of vm_pgoff fixes, see
http://marc.theaimsgroup.com/?l=linux-kernel&m=108938233708584

Oleg.

Signed-off-by: Oleg Nesterov <oleg@tv-sign.ru>

--- 2.6.7-mm7/fs/hugetlbfs/inode.c.pgoff	2004-07-11 15:32:09.000000000 +0400
+++ 2.6.7-mm7/fs/hugetlbfs/inode.c	2004-07-11 16:09:27.000000000 +0400
@@ -52,6 +52,9 @@ static int hugetlbfs_file_mmap(struct fi
 	loff_t len, vma_len;
 	int ret;
 
+	if ((vma->vm_flags & (VM_MAYSHARE | VM_WRITE)) == VM_WRITE)
+		return -EINVAL;
+
 	if (vma->vm_pgoff & (HPAGE_SIZE / PAGE_SIZE - 1))
 		return -EINVAL;
 
@@ -70,10 +73,19 @@ static int hugetlbfs_file_mmap(struct fi
 	file_accessed(file);
 	vma->vm_flags |= VM_HUGETLB | VM_RESERVED;
 	vma->vm_ops = &hugetlb_vm_ops;
+
+	ret = -ENOMEM;
+	len = vma_len + ((loff_t)vma->vm_pgoff << PAGE_SHIFT);
+	if (!(vma->vm_flags & VM_WRITE) && len > inode->i_size)
+		goto out;
+
 	ret = hugetlb_prefault(mapping, vma);
-	len = vma_len +	((loff_t)vma->vm_pgoff << PAGE_SHIFT);
-	if (ret == 0 && inode->i_size < len)
+	if (ret)
+		goto out;
+
+	if (inode->i_size < len)
 		inode->i_size = len;
+out:
 	up(&inode->i_sem);
 
 	return ret;

Re: [PATCH] hugetlbfs private mappings.

[William Lee Irwin III]

On Sun, Jul 11, 2004 at 04:59:38PM +0400, Oleg Nesterov wrote:
> Hugetlbfs silently coerce private mappings of hugetlb files
> into shared ones. So private writable mapping has MAP_SHARED
> semantics. I think, such mappings should be disallowed.
> First, such behavior allows open hugetlbfs file O_RDONLY, and
> overwrite it via mmap(PROT_READ|PROT_WRITE, MAP_PRIVATE), so
> it is security bug.
> Second, private writable mmap() should fail just because kernel
> does not support this.
> I beleive, it is ok to allow private readonly hugetlb mappings,
> sys_mprotect() does not work with hugetlb vmas.
> There is another problem. Hugetlb mapping is always prefaulted,
> pages allocated at mmap() time. So even readonly mapping allows
> to enlarge the size of the hugetlbfs file, and steal huge pages
> without appropriative permissions.
> Patch on top of vm_pgoff fixes, see
> http://marc.theaimsgroup.com/?l=linux-kernel&m=108938233708584

This probably doesn't break anything worth caring about, but it may
make people happier to just force MAP_SHARED on.


-- wli

Re: [PATCH] hugetlbfs private mappings.

[Oleg Nesterov]

William Lee Irwin III wrote:
> 
> On Sun, Jul 11, 2004 at 04:59:38PM +0400, Oleg Nesterov wrote:
> > Hugetlbfs silently coerce private mappings of hugetlb files
> > into shared ones. So private writable mapping has MAP_SHARED
> > semantics. I think, such mappings should be disallowed.
> 
> This probably doesn't break anything worth caring about, but it may
> make people happier to just force MAP_SHARED on.

Yes, it was my initial intent. Andrew Morton pointed out, that
this could break existing applications.

Oleg.

Re: [PATCH] hugetlbfs private mappings.

[William Lee Irwin III]

William Lee Irwin III wrote:
>> This probably doesn't break anything worth caring about, but it may
>> make people happier to just force MAP_SHARED on.

On Mon, Jul 12, 2004 at 04:11:18PM +0400, Oleg Nesterov wrote:
> Yes, it was my initial intent. Andrew Morton pointed out, that
> this could break existing applications.

I see; I thought the concern went the other direction. All's well, then.


-- wli