[Lguest] [PATCH v2 1/3] lguest: move the initial guest page table creation code to the host

[Rusty Russell]

On Monday 29 September 2008 14:39:55 Matias Zabaljauregui wrote:
> This patch moves the initial guest page table creation code to the host,
> so the launcher keeps working with PAE enabled configs.

OK, applied with some fixes.

> * the base of Guest "physical" memory, the top physical page to allow, the
> * top level pagetable and the entry point for the Guest. */ 

Fixed this comment, and similarly the one from the header.

> +static unsigned long setup_pagetables(unsigned long mem,
> +				      unsigned long initrd_size,
> +				      unsigned long mem_base)

I found it clearer to pass in lg here, and use lg->mem_base in this function.

> +{
> +	pgd_t *pgdir;
> +	pte_t *linear;

These are actually userspace pointers, so changed to:

+	pgd_t __user *pgdir;
+	pte_t __user *linear;

> +	/* Linear mapping is easy: put every page's address into the
> +	 * mapping in order. */
> +	for (i = 0; i < mapped_pages; i++)
> +		set_pte(&linear[i], pfn_pte(i,
> +			__pgprot(_PAGE_PRESENT | _PAGE_RW | _PAGE_USER)));

This direct access to userspace mem is a no-no.  Here's what I have now:

+	for (i = 0; i < mapped_pages; i++) {
+		pte_t pte;
+		pte = pfn_pte(i, __pgprot(_PAGE_PRESENT|_PAGE_RW|_PAGE_USER));
+		if (copy_to_user(&linear[i], &pte, sizeof(pte)) != 0)
+			return -EFAULT;
+	}

> +	/* The top level points to the linear page table pages above.
> +	 * We setup the identity and linear mappings here. */
> +	phys_linear = (unsigned long)linear - mem_base;
> +	for (i = 0; i < mapped_pages; i += PTRS_PER_PTE)
> +		pgdir[i / PTRS_PER_PTE] =
> +			pgdir[pgd_index(PAGE_OFFSET) + i / PTRS_PER_PTE] =
> +			     __pgd((phys_linear + i * sizeof(pte_t)) |
> +				 (_PAGE_PRESENT | _PAGE_RW | _PAGE_USER));

Same issue here.

+	for (i = 0; i < mapped_pages; i += PTRS_PER_PTE) {
+		pgd_t pgd;
+		pgd = __pgd((phys_linear + i * sizeof(pte_t)) |
+			    (_PAGE_PRESENT | _PAGE_RW | _PAGE_USER));
+
+		if (copy_to_user(&pgdir[i / PTRS_PER_PTE], &pgd, sizeof(pgd))
+		    || copy_to_user(&pgdir[pgd_index(PAGE_OFFSET)
+					   + i / PTRS_PER_PTE],
+				    &pgd, sizeof(pgd)))
+			return -EFAULT;
+	}
+

> +	unsigned long initrd_size = 0, mem = 0;
> +	struct boot_params *boot = (struct boot_params *) lg->mem_base;
> +
> +	/* Get the guest memory size and the ramdisk size
> +	 * from the boot header located at lg->mem_base*/
> +	if (copy_from_user(&mem, &boot->e820_map[0].size, 8))
> +		kill_guest(&lg->cpus[0], "Error reading from user space");

This 8 here caused a stack issue (lg got set to NULL in the caller function)!
I changed mem to a u64.

> +	lg->pgdirs[0].gpgdir = setup_pagetables(mem,
> +				initrd_size, (unsigned long) lg->mem_base);

This can now error, so I did:

+	lg->pgdirs[0].gpgdir = setup_pagetables(lg, mem, initrd_size);
+	if (IS_ERR_VALUE(lg->pgdirs[0].gpgdir))
+		return lg->pgdirs[0].gpgdir;

Cheers!
Rusty.