8

I've been doing some research and I'm a little confused about this macro. Hopefully someone can give me some guidance. I have some ioctl code (which I've inherited, not written) and the first thing it does if check if access_ok() before moving on to copying data over from user space:

#define __lddk_copy_from_user(a,b,c) copy_from_user(a,b,c)
#define __lddk_copy_to_user(a,b,c) copy_to_user(a,b,c)

long can_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
{
  switch(cmd) {
    case COMMAND:
      if(! access_ok(VERIFY_READ, (void *)arg, sizeof(Message_par_t)))
        return(retval); 

      if(! access_ok(VERIFY_WRITE, (void *)arg, sizeof(Message_par_t)))
        return(retval); 

      argp = &Command;
      __lddk_copy_from_user( (void *) argp,(Command_par_t *) arg, sizeof(Command_par_t));

So the code works just fine, but I'm not sure it's needed. The first question comes from this description of access_ok's return:

  • The function returns non-zero if the region is likely accessible (though access may still result in -EFAULT). This function simply checks that the address is likely in user space, not in the kernel.

So this means that it really does nothing more then make sure the pointer we're checking against is probably initialized in user space? Since we know that we couldn't come into this function other than a user space call, and it couldn't happen unless we opened a valid file descriptor to this device, is this really needed? Is it really any safer then just making sure we didn't get a NULL pointer?

Second question comes from this description:

  • The type argument can be specified as VERIFY_READ or VERIFY_WRITE. The VERIFY_WRITE symbolic also identifies whether the memory region is readable as well as writable.

Does this mean the first check in my code is redundant? If we're going to check for a writable area we get readable as a freebie?

I'm using a x86 architecture so the definations of access_ok() and __range_no_ok() are those from /usr/src/linux-3.1.10-1.16/arch/x86/include/asm/uaccess.h as follows:

#define access_ok(type, addr, size) (likely(__range_not_ok(addr, size) == 0))

#define __range_not_ok(addr, size)                  \
({                                  \
    unsigned long flag, roksum;                 \
    __chk_user_ptr(addr);                       \
    asm("add %3,%1 ; sbb %0,%0 ; cmp %1,%4 ; sbb $0,%0"     \
        : "=&r" (flag), "=r" (roksum)               \
        : "1" (addr), "g" ((long)(size)),               \
          "rm" (current_thread_info()->addr_limit.seg));        \
    flag;                               \
})
Mike
  • 47,263
  • 29
  • 113
  • 177
  • I'm no Linux kernel expert, but, as a rule of thumb, normally I think it's better to just try the operation and let the function fail instead of performing those checks in advance *still* having to be prepared for a failure (unless there's some performance reason why performing a check in advance may save a lot of time); also, checking the source it looks like `copy_from_user` already performs the read check anyway, so it's redundant. Just check its return value (=number of bytes that couldn't be copied), if it's different than zero something failed and you have to return an error. – Matteo Italia Sep 10 '12 at 19:09
  • 1
    The answer to this depends on what the `__lddk_copy_from_user()` function/macro does - that's not in the mainstream kernel. Can you update your question with the definition of this function? – caf Sep 11 '12 at 01:43
  • @caf - good point, this is the "standard" uCLinux can driver, and that wrapper is defined in can_defs.h, but yeah, I had to look it up before too not knowing what it was. – Mike Sep 11 '12 at 11:31

3 Answers3

14

If __lddk_copy_from_user() simply calls copy_from_user(), then the access_ok() checks are redundant, because copy_from_user() performs these checks itself.

The access_ok() checks ensure that the userspace application isn't asking the kernel to read from or write to kernel addresses (they're an integrity/security check). Just because a pointer was supplied by userspace doesn't mean that it's definitely a userspace pointer - in many cases "kernel pointer" simply means that it's pointing within a particular region of the virtual address space.

Additionally, calling access_ok() with VERIFY_WRITE implies VERIFY_READ, so if you check the former you do not need to also check the latter.

As of this commit in 2019, access_ok() no long has the type argument, so the VERIFY_WRITE versus VERIFY_READ point is moot.
caf
  • 233,326
  • 40
  • 323
  • 462
1

The access_ok macro is just a quick check for the probable validity of the pointer. For example, it would catch mistaken calls with NULL or read-only arguments.

Ideally, the driver should recover even if later the access functions fail. This could however happen only after some expensive hardware operations. Therefore checking early may make the driver more robust against the most common user-space programmer errors.

Edit: and yeah, if your only input call is the copy_from_user() there, the checks are redundant. However, if there is a write later on, it is useful to check the writability right at the beginning.

And you should really check the return value of copy_from_user().

jpa
  • 10,351
  • 1
  • 28
  • 45
  • Could you explain to me how it "catches" calls with a NULL pointer? I see `__range_not_ok()` calls `__chk_user_ptr()` so it's probably in there, but I can't find where that's defined. – Mike Sep 11 '12 at 11:42
  • 1
    I don't believe this is true, it only checks if it's in the userspace address range in x86. Null pointers are within that range and so would pass. – jleahy Sep 12 '12 at 15:33
1

It's not redundant. access_ok() validates the address, attempting to read from this region. If the address is valid and does exist in user space, then it tries to read, otherwise the function returns EFAULT, which indicates an address failure. It is a safer way to verify access on regions rather than checking against NULL (before you get any segmentation fault which may cause your kernel to crash).

Moreover you can verify read/write access, using access_ok(). The second call just verifies "write" access on that region.

Erhan Bagdemir
  • 5,231
  • 6
  • 34
  • 40
  • So your comment is that what I had read, namely, `VERIFY_WRITE symbolic also identifies whether the memory region is readable as well as writable.` is an inaccurate statement? Thus an access_ok(read) is still useful even if an access_ok(write) will be called? – Mike Sep 10 '12 at 19:29
  • it is not inaccurate. you must be sure whether you've the "rights" to access on a memory region. it can be possible that you've just read rights or write, or even both. – Erhan Bagdemir Sep 11 '12 at 08:29