From 2d37f94a28170ca656438758fca577acb49a7932 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Mon, 22 Oct 2007 11:24:24 +1000 Subject: [PATCH] generalize lgread_u32/lgwrite_u32. Jes complains that page table code still uses lgread_u32 even though it now uses general kernel pte types. The best thing to do is to generalize lgread_u32 and lgwrite_u32. This means we lose the efficiency of getuser(). We could potentially regain it if we used __copy_from_user instead of copy_from_user, but I'm not certain that our range check is equivalent to access_ok() on all platforms. Signed-off-by: Rusty Russell Acked-by: Jes Sorensen --- drivers/lguest/core.c | 39 ++++++--------------------- drivers/lguest/hypercalls.c | 2 +- drivers/lguest/interrupts_and_traps.c | 2 +- drivers/lguest/lg.h | 23 +++++++++++++--- drivers/lguest/page_tables.c | 10 +++---- drivers/lguest/segments.c | 4 +-- drivers/lguest/x86/core.c | 4 +-- 7 files changed, 38 insertions(+), 46 deletions(-) diff --git a/drivers/lguest/core.c b/drivers/lguest/core.c index 3aec29ec7715..35d19ae58de7 100644 --- a/drivers/lguest/core.c +++ b/drivers/lguest/core.c @@ -145,33 +145,10 @@ int lguest_address_ok(const struct lguest *lg, return (addr+len) / PAGE_SIZE < lg->pfn_limit && (addr+len >= addr); } -/* This is a convenient routine to get a 32-bit value from the Guest (a very - * common operation). Here we can see how useful the kill_lguest() routine we - * met in the Launcher can be: we return a random value (0) instead of needing - * to return an error. */ -u32 lgread_u32(struct lguest *lg, unsigned long addr) -{ - u32 val = 0; - - /* Don't let them access lguest binary. */ - if (!lguest_address_ok(lg, addr, sizeof(val)) - || get_user(val, (u32 *)(lg->mem_base + addr)) != 0) - kill_guest(lg, "bad read address %#lx: pfn_limit=%u membase=%p", addr, lg->pfn_limit, lg->mem_base); - return val; -} - -/* Same thing for writing a value. */ -void lgwrite_u32(struct lguest *lg, unsigned long addr, u32 val) -{ - if (!lguest_address_ok(lg, addr, sizeof(val)) - || put_user(val, (u32 *)(lg->mem_base + addr)) != 0) - kill_guest(lg, "bad write address %#lx", addr); -} - -/* This routine is more generic, and copies a range of Guest bytes into a - * buffer. If the copy_from_user() fails, we fill the buffer with zeroes, so - * the caller doesn't end up using uninitialized kernel memory. */ -void lgread(struct lguest *lg, void *b, unsigned long addr, unsigned bytes) +/* This routine copies memory from the Guest. Here we can see how useful the + * kill_lguest() routine we met in the Launcher can be: we return a random + * value (all zeroes) instead of needing to return an error. */ +void __lgread(struct lguest *lg, void *b, unsigned long addr, unsigned bytes) { if (!lguest_address_ok(lg, addr, bytes) || copy_from_user(b, lg->mem_base + addr, bytes) != 0) { @@ -181,15 +158,15 @@ void lgread(struct lguest *lg, void *b, unsigned long addr, unsigned bytes) } } -/* Similarly, our generic routine to copy into a range of Guest bytes. */ -void lgwrite(struct lguest *lg, unsigned long addr, const void *b, - unsigned bytes) +/* This is the write (copy into guest) version. */ +void __lgwrite(struct lguest *lg, unsigned long addr, const void *b, + unsigned bytes) { if (!lguest_address_ok(lg, addr, bytes) || copy_to_user(lg->mem_base + addr, b, bytes) != 0) kill_guest(lg, "bad write address %#lx len %u", addr, bytes); } -/* (end of memory access helper routines) :*/ +/*:*/ /*H:030 Let's jump straight to the the main loop which runs the Guest. * Remember, this is called by the Launcher reading /dev/lguest, and we keep diff --git a/drivers/lguest/hypercalls.c b/drivers/lguest/hypercalls.c index 3a53788ba450..9d5184c7c14a 100644 --- a/drivers/lguest/hypercalls.c +++ b/drivers/lguest/hypercalls.c @@ -47,7 +47,7 @@ static void do_hcall(struct lguest *lg, struct hcall_args *args) char msg[128]; /* If the lgread fails, it will call kill_guest() itself; the * kill_guest() with the message will be ignored. */ - lgread(lg, msg, args->arg1, sizeof(msg)); + __lgread(lg, msg, args->arg1, sizeof(msg)); msg[sizeof(msg)-1] = '\0'; kill_guest(lg, "CRASH: %s", msg); break; diff --git a/drivers/lguest/interrupts_and_traps.c b/drivers/lguest/interrupts_and_traps.c index 3271c0031a1b..82966982cb38 100644 --- a/drivers/lguest/interrupts_and_traps.c +++ b/drivers/lguest/interrupts_and_traps.c @@ -45,7 +45,7 @@ static void push_guest_stack(struct lguest *lg, unsigned long *gstack, u32 val) { /* Stack grows upwards: move stack then write value. */ *gstack -= 4; - lgwrite_u32(lg, *gstack, val); + lgwrite(lg, *gstack, u32, val); } /*H:210 The set_guest_interrupt() routine actually delivers the interrupt or diff --git a/drivers/lguest/lg.h b/drivers/lguest/lg.h index 4d45b7036e82..d9144beca82c 100644 --- a/drivers/lguest/lg.h +++ b/drivers/lguest/lg.h @@ -98,12 +98,27 @@ struct lguest extern struct mutex lguest_lock; /* core.c: */ -u32 lgread_u32(struct lguest *lg, unsigned long addr); -void lgwrite_u32(struct lguest *lg, unsigned long addr, u32 val); -void lgread(struct lguest *lg, void *buf, unsigned long addr, unsigned len); -void lgwrite(struct lguest *lg, unsigned long, const void *buf, unsigned len); int lguest_address_ok(const struct lguest *lg, unsigned long addr, unsigned long len); +void __lgread(struct lguest *, void *, unsigned long, unsigned); +void __lgwrite(struct lguest *, unsigned long, const void *, unsigned); + +/*L:306 Using memory-copy operations like that is usually inconvient, so we + * have the following helper macros which read and write a specific type (often + * an unsigned long). + * + * This reads into a variable of the given type then returns that. */ +#define lgread(lg, addr, type) \ + ({ type _v; __lgread((lg), &_v, (addr), sizeof(_v)); _v; }) + +/* This checks that the variable is of the given type, then writes it out. */ +#define lgwrite(lg, addr, type, val) \ + do { \ + typecheck(type, val); \ + __lgwrite((lg), (addr), &(val), sizeof(val)); \ + } while(0) +/* (end of memory access helper routines) :*/ + int run_guest(struct lguest *lg, unsigned long __user *user); /* Helper macros to obtain the first 12 or the last 20 bits, this is only the diff --git a/drivers/lguest/page_tables.c b/drivers/lguest/page_tables.c index fe3c7575647b..2a45f0691c9b 100644 --- a/drivers/lguest/page_tables.c +++ b/drivers/lguest/page_tables.c @@ -209,7 +209,7 @@ int demand_page(struct lguest *lg, unsigned long vaddr, int errcode) pte_t *spte; /* First step: get the top-level Guest page table entry. */ - gpgd = __pgd(lgread_u32(lg, gpgd_addr(lg, vaddr))); + gpgd = lgread(lg, gpgd_addr(lg, vaddr), pgd_t); /* Toplevel not present? We can't map it in. */ if (!(pgd_flags(gpgd) & _PAGE_PRESENT)) return 0; @@ -235,7 +235,7 @@ int demand_page(struct lguest *lg, unsigned long vaddr, int errcode) /* OK, now we look at the lower level in the Guest page table: keep its * address, because we might update it later. */ gpte_ptr = gpte_addr(lg, gpgd, vaddr); - gpte = __pte(lgread_u32(lg, gpte_ptr)); + gpte = lgread(lg, gpte_ptr, pte_t); /* If this page isn't in the Guest page tables, we can't page it in. */ if (!(pte_flags(gpte) & _PAGE_PRESENT)) @@ -278,7 +278,7 @@ int demand_page(struct lguest *lg, unsigned long vaddr, int errcode) /* Finally, we write the Guest PTE entry back: we've set the * _PAGE_ACCESSED and maybe the _PAGE_DIRTY flags. */ - lgwrite_u32(lg, gpte_ptr, pte_val(gpte)); + lgwrite(lg, gpte_ptr, pte_t, gpte); /* We succeeded in mapping the page! */ return 1; @@ -366,12 +366,12 @@ unsigned long guest_pa(struct lguest *lg, unsigned long vaddr) pte_t gpte; /* First step: get the top-level Guest page table entry. */ - gpgd = __pgd(lgread_u32(lg, gpgd_addr(lg, vaddr))); + gpgd = lgread(lg, gpgd_addr(lg, vaddr), pgd_t); /* Toplevel not present? We can't map it in. */ if (!(pgd_flags(gpgd) & _PAGE_PRESENT)) kill_guest(lg, "Bad address %#lx", vaddr); - gpte = __pte(lgread_u32(lg, gpte_addr(lg, gpgd, vaddr))); + gpte = lgread(lg, gpte_addr(lg, gpgd, vaddr), pte_t); if (!(pte_flags(gpte) & _PAGE_PRESENT)) kill_guest(lg, "Bad address %#lx", vaddr); diff --git a/drivers/lguest/segments.c b/drivers/lguest/segments.c index 95eb9cf297bf..c2434ec99f7b 100644 --- a/drivers/lguest/segments.c +++ b/drivers/lguest/segments.c @@ -150,7 +150,7 @@ void load_guest_gdt(struct lguest *lg, unsigned long table, u32 num) kill_guest(lg, "too many gdt entries %i", num); /* We read the whole thing in, then fix it up. */ - lgread(lg, lg->arch.gdt, table, num * sizeof(lg->arch.gdt[0])); + __lgread(lg, lg->arch.gdt, table, num * sizeof(lg->arch.gdt[0])); fixup_gdt_table(lg, 0, ARRAY_SIZE(lg->arch.gdt)); /* Mark that the GDT changed so the core knows it has to copy it again, * even if the Guest is run on the same CPU. */ @@ -161,7 +161,7 @@ void guest_load_tls(struct lguest *lg, unsigned long gtls) { struct desc_struct *tls = &lg->arch.gdt[GDT_ENTRY_TLS_MIN]; - lgread(lg, tls, gtls, sizeof(*tls)*GDT_ENTRY_TLS_ENTRIES); + __lgread(lg, tls, gtls, sizeof(*tls)*GDT_ENTRY_TLS_ENTRIES); fixup_gdt_table(lg, GDT_ENTRY_TLS_MIN, GDT_ENTRY_TLS_MAX+1); lg->changed |= CHANGED_GDT_TLS; } diff --git a/drivers/lguest/x86/core.c b/drivers/lguest/x86/core.c index ef976ccb4192..9eed12d5a395 100644 --- a/drivers/lguest/x86/core.c +++ b/drivers/lguest/x86/core.c @@ -222,7 +222,7 @@ static int emulate_insn(struct lguest *lg) return 0; /* Decoding x86 instructions is icky. */ - lgread(lg, &insn, physaddr, 1); + insn = lgread(lg, physaddr, u8); /* 0x66 is an "operand prefix". It means it's using the upper 16 bits of the eax register. */ @@ -230,7 +230,7 @@ static int emulate_insn(struct lguest *lg) shift = 16; /* The instruction is 1 byte so far, read the next byte. */ insnlen = 1; - lgread(lg, &insn, physaddr + insnlen, 1); + insn = lgread(lg, physaddr + insnlen, u8); } /* We can ignore the lower bit for the moment and decode the 4 opcodes -- 2.20.1