Source code example from "Linux kernel programming"

Question

was reading Robert Love's book, chapter 5 on syscalls, and found this simple example a bit questionable:

asmlinkage long sys_silly_copy(unsigned long *src, unsigned long *dst, unsigned long len)
{
   unsigned long buf;

   if (copy_from_user(&buf, src, len))
     return -EFAULT;

   ...
}

As we see 'buf' is object of type 'unsigned long' and defined on the kernel stack, i.e. its initial value is likely garbage. Anyway is it valid to copy 'len' bytes in the stack where buf is, i.e. it could overwrite something useful? Perhaps this is fine only for this particular example?

Answer 1

It is very questionable. In fact, it's downright dangerous. I'll give the author the benefit of the doubt here since they're just trying to show how copy_from_user and copy_to_user work but they really should have provided an example that wasn't so dangerous.

Especially since the book waxes lyrical about how you must be extra careful:

System calls must carefully verify all their parameters to ensure that they are valid and legal.The system call runs in kernel-space, and if the user can pass invalid input into the kernel without restraint, the system’s security and stability can suffer.

and then provides a means for the user to totally annihilate the kernel :-)

---

The text from the copy I have states:

Let’s consider an example system call that uses both copy_from_user() and copy_to_user().This syscall, silly_copy(), is utterly worthless; it copies data from its first parameter into its second.This is suboptimal in that it involves an intermediate and extraneous copy into kernel-space for no gain. But it helps illustrate the point.

/*
* silly_copy - pointless syscall that copies the len bytes from
* ‘src’ to ‘dst’ using the kernel as an intermediary in the copy.
* Intended as an example of copying to and from the kernel.
*/
SYSCALL_DEFINE3(silly_copy,
                unsigned long *, src,
                unsigned long *, dst,
                unsigned long len)
{
    unsigned long buf;

    /* copy src, which is in the user’s address space, into buf */
    if (copy_from_user(&buf, src, len))
        return -EFAULT;

    /* copy buf into dst, which is in the user’s address space */
    if (copy_to_user(dst, &buf, len))
        return -EFAULT;

    /* return amount of data copied */
    return len;
}

---

Other than the catastrophic failure of not checking parameters, I'm pretty certain the last parameter of the SYSCALL_DEFINE3 is missing a comma (though that would just be a typo).

A far better example, without having to allocate arbitrary memory, would be along the lines of:

SYSCALL_DEFINE3(silly_copy,
                unsigned long *, src,
                unsigned long *, dst,
                unsigned long,   len)
{
    unsigned long buf[64];                 /* Buffer for chunks */
    unsigned long lenleft = len;           /* Remaining size */
    unsigned long chunklen = sizeof(buf);  /* Initial chunk length */

    /* Loop handling chunk sizes */

    while (lenleft > 0) {
        /* Change chunk length on last chunk */

        if (lenleft < chunklen) chunklen = lenleft;

        /* copy src(user) to buf(kernel) then dst(user) */

        if (copy_from_user(buf, src, chunklen)) return -EFAULT;
        if (copy_to_user(dst, buf, chunklen)) return -EFAULT;

        /* Adjust pointers and remaining size */

        src += chunklen; dst += chunklen; lenleft -= chunklen;
    }

    /* return amount of data copied */
    return len;
}

---

Anyone trying to implement that system call would be well advised to steer away from that particular sample in the book, although I suppose, at a bare minimum, it will give you some good kernel debugging experience :-)