Re: [PATCH 2/2] lib: Limit strnlen_user() return value to count + 1

[Linus Torvalds]

On Wed, Jun 3, 2015 at 2:21 AM, Jan Kara <jack@xxxxxxx> wrote:
>
> The comment is:

Yeah, and the comment right above do_strnlen_user() that you ignored is:

 * NOTE! We can sometimes overshoot the user-supplied maximum
 * if it fits in a aligned 'long'. The caller needs to check
 * the return value against "> max".

Which is pretty unambiguous.

The thing is, "retval > max" shiould be considered an error condition.
Exactly like 0 is, and the caller should check for that.

I do agree that we should change the other comment too, though. I
think there may have been some cutting-and-pasting when the code was

> What they roughly did was:
>
>         char buf[DM_ATTR_NAME_SIZE + 1];
>
>         len = strnlen_user(from, DM_ATTR_NAME_SIZE);
>         if (!len)
>                 return -EFAULT;
>         if (copy_from_user(buf, from, len))
>                 return -EFAULT;
>         buf[len - 1] = 0;

Yeah, don't do that.

It's stupid code anyway.

If what you wanted was "strncpy_from_user()", that's what you should have used.

That function actually takes care to be exact, because it obviously
has a destination buffer that it really cannot overshoot.

So

        char buf[DM_ATTR_NAME_SIZE + 1];

        if (strncpy_from_user(buf, from, len) < 0)
            return -EFAULT;
        buf[DM_ATTR_NAME_SIZE] = 0;

should actually work.

[ Side note: the generic strncpy_from_user() routine can be
inefficient on architectures that handle unaligned accesses badly, but
considering that it's used for copying pathnames from user space, I
hope such architectures have their own optimized version ]

I actually would like to get rid of "strnlen_user()" users as much as
humanly possible. It's a fundamentally racy interface, since we don't
control user memory, and another thread could change the string as it
is being counted. There are cases where we have to use it (execve
argument handling is I think the only real case of "yeah, we have no
alternatives"), so we can't get rid of it entirely, but I basically
don't believe in trying to make that interface at all easier to use.

I'd almost be inclined to unexport it. From a quick look, we don't
have any module users.

                  Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/