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?

Looks fine as long as len == sizeof(unsigned long). All that's happening is len bytes from src are being written into buf. Just don't len to be too big or the ret address and other important structures could be overwritten — flumpb, Jun 29 '11 at 02:52
@kisplit: you _never_ rely on your callers following the rules, _especially_ if you're the kernel. If you wanted to ensure the copy was only for a single ulong, you'd use `sizeof(unsigned long)` in the copy operations rather than `len`. I've provided a more secure version in my answer. — paxdiablo, Jun 29 '11 at 03:25
@paxdiablo: I totally agree. Of course it's dangerous unless len == sizeof(unsigned long) in this specific example. — flumpb, Jun 29 '11 at 05:30

paxdiablo · Accepted Answer · 2011-06-29T14:13:47.450

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 :-)

thanks for your comments! In your example, should not it be copy_from_user(buf, src, chunklen) instead of copy_from_user(&buf, src, chunklen), i.e. it is sufficient to supply a pointer to the first element of the array and the length? — Mark, Jun 29 '11 at 12:50
@Mark, yes, you're right, I've changed it to the correct form. — paxdiablo, Jun 29 '11 at 13:55

score -1 · Answer 2 · edited Nov 22 '14 at 14:53

int init_module(void)
{

    mempool_t *mempool;

    struct kmem_cache *kmem_cache;
    void *p0 , *p1;
    kmem_cache = kmem_cache_create("Ashrama" ,100 , 0 ,SLAB_PANIC ,NULL);
    mempool = mempool_create(4 , mempool_alloc_slab , mempool_free_slab , kmem_cache);
    p0 = mempool_alloc(mempool, SLAB_PANIC);
    p1 = mempool_alloc(mempool , SLAB_PANIC);
    strcpy(p0 , "Ranjan.B.M");
    strcpy(p1 , "Mithun.V");
    mempool_free( p0 , mempool);
    printk(KERN_ALERT"%s",p0);
    printk(KERN_ALERT"%s",p1);
}

Source code example from "Linux kernel programming"

2 Answers2

Linked