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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CA+55aFyndqgESxJgsYjXHhu82GBb4Y5ojPvJv4oWL_5iw3xSPw@mail.gmail.com>
Date:	Wed, 3 Jun 2015 06:21:45 -0700
From:	Linus Torvalds <torvalds@...ux-foundation.org>
To:	Jan Kara <jack@...e.cz>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/2] lib: Limit strnlen_user() return value to count + 1

On Wed, Jun 3, 2015 at 2:21 AM, Jan Kara <jack@...e.cz> 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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/