On 2015-10-31 at 09:18 "'Davide Libenzi' via Akaros"
<[email protected]> wrote:
> I have also added the strcpy to/from user, but we seem to pass the
> length from userspace AFAICS.
> Up to you if you think they will become handy or not.
Overall, I like replacing memcpy_{to,from}_user, but I think the string
ops have some issues. More below:
> From 33ee96c0726fe928e3219a12cff819cac9ad131f Mon Sep 17 00:00:00 2001
> From: Davide Libenzi <[email protected]>
> Date: Fri, 30 Oct 2015 18:31:15 -0700
> Subject: Migrated user memory copy APIs to use the new exception table guards
>
> Migrated user memory copy APIs to use the new exception table guards.
> Added string copy APIs as well.
>
> Signed-off-by: Davide Libenzi <[email protected]>
> ---
> kern/arch/x86/uaccess.h | 98 ++++++++++++++-----------
Minor thing, but anytime we add a arch-specific header that gets used in
arch-independent code (as this one is later), please add a header with
stubs to the other architectures.
> diff --git a/kern/src/umem.c b/kern/src/umem.c
> index 87f4e3a1e332..8038653918fb 100644
> --- a/kern/src/umem.c
> +++ b/kern/src/umem.c
> @@ -7,6 +7,7 @@
> +static int string_copy_from_user(char *dst, const char *src)
> {
> - const void *start, *end;
> - size_t num_pages, i;
> - pte_t pte;
> - uintptr_t perm = PTE_USER_RO;
> - size_t bytes_copied = 0;
> + if (unlikely(!is_user_raddr((void *) src, 1)))
> + return -EFAULT;
Is this unlikely() necessary? We already have the unlikely() inside
is_user_raddr, which is static inlined. I'd like to limit the
unlikely() calls a bit.
More importantly, what happens if the user picks an address close to the
border of the an area where it does not have access, but the string goes
into that area. For instance, in copy_to_user, I think the user could
pick UWLIM - 1, then trick the kernel into writing above that into
memory that is read-only.
> + for (;; dst++, src++) {
> + int error = __get_user(dst, src, 1);
>
> - static_assert(ULIM % PGSIZE == 0 && ULIM != 0); // prevent wrap-around
> + if (unlikely(error))
> + return error;
> + if (unlikely(!*dst))
> + break;
How unlikely is this? It'll happen once per strcpy, right? Is that
going to be a TLB miss or something based on what the compiler does to
make something unlikely? This example gets at the tradeoff of using
likely/unlikely.
> +int memcpy_to_user(struct proc *p, void *dest, const void *src, size_t len)
> +{
> + struct proc *prev = switch_to(p);
> + int error = copy_to_user(dest, src, len);
> +
> + switch_back(p, prev);
> +
> + return error;
> +}
This is great; I'm a huge fan of the memcpy_to_user cleanups.
Barret
--
You received this message because you are subscribed to the Google Groups
"Akaros" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To post to this group, send email to [email protected].
For more options, visit https://groups.google.com/d/optout.
