I'm back - glibc LDT handling (was: Re: [uml-devel] I'm out for some time - SKAS host leak diagnosis.)

[BlaisorBlade <blaisorblade_spam@yahoo.it>]

[-- Attachment #1: Type: text/plain, Size: 2829 bytes --]

Alle 18:04, giovedì 8 luglio 2004, BlaisorBlade ha scritto:
> 1) From 9 to 18 July, I'll be away for study 
Ok, I'm back. And glad to see the release of 2.4.26-2um.

> 2) Especially, I'm near to fixing the host SKAS leak... seems like it's a
> race condition when calling alloc_ldt(): one of the two values is
> overwritten by the other, and so never freed. In fact for each mm
> init_new_context() is called when open()ing it, while __init_new_context()
> is called when writing a MM_COPY_SEGMENTS request onto /proc/mm (which
> happens shortly after). The mm->context is not locked anyway, and this is a
> SKAS bug: it makes sense in mainline, but not when using SKAS. The strange
> thing is that on host/kernel configurations where it happens, it is
> repeatable, i.e. does not seem a race condition. However the locking must
> be anyway added.

I've attached the patch, but I've not had time to even compile one kernel with 
it. However try it if you want: it's for 2.6 and probably works for 2.4.

Ok, the problem is the double call, but it is not a race condition: when 
calling again __init_new_context, we do mm->context.size = 0, so alloc_ldt 
does not free mm->context.ldt. Much simpler. While the race condition is 
impossible (it is the same thread to open /proc/mm and to write the 
MM_COPY_SEGMENTS request).

And it did not happen before 2.4.25 because there was no clearing of 
mm->context.size (it did not exist). There is another strange thing: why the 
leak does not happen everywhere? To actually have an LDT to allocate, the UML 
host thread calling new_mm() must have an LDT with size != 0, which will then 
be inherited by every UML thread: I do not think this is nice, and it's 
unwanted since UML assumes that a kernel thread has not an LDT, see the 
current->mm != NULL check in new_mm(); I think this can only come from glibc 
(for instance to support TLS).

Anyway, that is the problem. And the locking is not the solution (deadlock 
risk): simply it does not make sense to call twice init_new_context(), 
because after the first time the context is old. I've avoided to add locking 
because I fear deadlocks (I would need to lock both the old and the new 
context) and because the old code worked fine without adding locks; anyway, 
there is no chance that a proper UML can cause a race onto the new LDT; but 
if a process wants to, it can. The fix would be very simple: lock the old 
context, copy its LDT to a bounce buffer, unlock it, and copy the bounce 
buffer to the new context under locking; but I don't like this. Maybe it 
could be deadlock-free to take the old_mm lock and after the new_mm one (it's 
impossible that another process does the reverse copy, or not?)

Bye
-- 
Paolo Giarrusso, aka Blaisorblade
Linux registered user n. 292729

[-- Attachment #2: skas-leak-fix.patch --]
[-- Type: text/x-diff, Size: 2755 bytes --]


init_new_context was called, and then __init_new_context; they both clear
the LDT (by setting its size to 0) and alloc a new one; and since the LDT size
has been cleared, alloc_ldt does not free() the LDT. It it exposed only if actually
the UML process has an LDT to allocate, i.e. if the UML kernel thread had an LDT
on the host when forking the init process.

Signed-off-by: Paolo 'Blaisorblade' Giarrusso <blaisorblade_spam@yahoo.it>
---

 include/asm-i386/mmu_context.h                        |    0 
 vanilla-linux-2.6.7-SKAS-paolo/arch/i386/kernel/ldt.c |   18 +++++++++++++++---
 vanilla-linux-2.6.7-SKAS-paolo/mm/proc_mm.c           |    3 ++-
 3 files changed, 17 insertions(+), 4 deletions(-)

diff -puN arch/i386/kernel/ldt.c~skas-leak-fix arch/i386/kernel/ldt.c
--- vanilla-linux-2.6.7-SKAS/arch/i386/kernel/ldt.c~skas-leak-fix	2004-07-19 13:52:46.439670608 +0200
+++ vanilla-linux-2.6.7-SKAS-paolo/arch/i386/kernel/ldt.c	2004-07-19 13:52:46.444669848 +0200
@@ -89,12 +89,14 @@ static inline int copy_ldt(mm_context_t 
  * we do not have to muck with descriptors here, that is
  * done in switch_mm() as needed.
  */
-int __init_new_context(struct mm_struct *mm, struct mm_struct *old_mm)
+int init_new_context(struct task_struct *tsk, struct mm_struct *mm)
 {
+	struct mm_struct * old_mm;
 	int retval = 0;
 
 	init_MUTEX(&mm->context.sem);
 	mm->context.size = 0;
+	old_mm = current->mm;
 	if (old_mm && old_mm->context.size > 0) {
 		down(&old_mm->context.sem);
 		retval = copy_ldt(&mm->context, &old_mm->context);
@@ -103,9 +105,19 @@ int __init_new_context(struct mm_struct 
 	return retval;
 }
 
-int init_new_context(struct task_struct *tsk, struct mm_struct *mm)
+int copy_context(struct mm_struct *mm, struct mm_struct *old_mm)
 {
-	return __init_new_context(mm, current->mm);
+	int err;
+	if (old_mm && old_mm->context.size > 0) {
+		down(&old_mm->context.sem);
+		err = alloc_ldt(new, old->size, 0);
+		if (err < 0)
+			goto out;
+		memcpy(new->ldt, old->ldt, old->size*LDT_ENTRY_SIZE);
+		up(&old_mm->context.sem);
+	}
+out:
+	return err;
 }
 
 /*
diff -puN mm/proc_mm.c~skas-leak-fix mm/proc_mm.c
--- vanilla-linux-2.6.7-SKAS/mm/proc_mm.c~skas-leak-fix	2004-07-19 13:52:46.441670304 +0200
+++ vanilla-linux-2.6.7-SKAS-paolo/mm/proc_mm.c	2004-07-19 13:52:46.445669696 +0200
@@ -12,6 +12,7 @@
 #include "asm/mmu_context.h"
 
 static struct file_operations proc_mm_fops;
+int copy_context(struct mm_struct *mm, struct mm_struct *old_mm);
 
 struct mm_struct *proc_mm_get_mm(int fd)
 {
@@ -93,7 +94,7 @@ static ssize_t write_proc_mm(struct file
 			break;
 		}
 
-		__init_new_context(mm, from);
+		copy_ldt(mm, from);
 		break;
 	}
 	default:
diff -puN include/asm-i386/mmu_context.h~skas-leak-fix include/asm-i386/mmu_context.h
_