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: <20150603092129.GE13054@quack.suse.cz>
Date:	Wed, 3 Jun 2015 11:21:29 +0200
From:	Jan Kara <jack@...e.cz>
To:	Linus Torvalds <torvalds@...ux-foundation.org>
Cc:	Jan Kara <jack@...e.cz>, 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 Tue 02-06-15 10:08:11, Linus Torvalds wrote:
> On Tue, Jun 2, 2015 at 8:10 AM, Jan Kara <jack@...e.cz> wrote:
> > Currently strnlen_user() can return numbers between 0 and
> > count + sizeof(unsigned long) - 1.
> 
> This is explicitly documented in the comment at the top of the function.

The comment is:

 * strnlen_user: - Get the size of a user string INCLUDING final NUL.
 * @str: The string to measure.
 * @count: Maximum count (including NUL character)
 *
 * Context: User context only.  This function may sleep.
 *
 * Get the size of a NUL-terminated string in user space.
 *
 * Returns the size of the string INCLUDING the terminating NUL.
 * If the string is too long, returns 'count+1'.
 * On exception (or invalid count), returns 0.

My interpretation of the sentence "If the string is too long, returns
'count+1'." is that the function will never return more than count+1. But
that's not true. So either we should clarify the comment or fix the
function.

> If there are out-of-tree users that don't check the return value
> correctly, then those out-of-tree users are buggy.
> 
> Why not fix the real bug? And why are you not talking about *which*
> out-of-tree user this is, and instead dancing around the issue.

The buggy user is XFS DMAPI patches from SGI we carry in SUSE kernel. And
yes, we have fixed those (actually SGI did, I just merged the patches from
them).

> So NAK on this. If you can actually convince me that the out-of-tree
> user has some valid reason for its obvious bug, then dammit, the
> comment at the top should also have been fixed.

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;

Which doesn't look that stupid to me (well, besides the fact that it's IMHO
better to return error than silently truncate the user provided string but
they do this for ages).

> But as it is, this is documented behavior and makes the code simpler,
> and I can't for the life of me see any possible valid reason why
> *anybody* could ever rely on anything but "retval > max". Which you
> *have* to check anyway. Exactly as documented.
> 
> In fact, maybe we should change that
> 
>         if (res >= count)
>                 return count+1;
> 
> do return "count < INT_MAX ? INT_MAX : count + 1" or something, to
> make sure nobody screws this up and doesn't try to use the value and
> depend on "count+1".
> 
> Basically strnlen_user() does *not* have the same semantics as
> "strlen()". Never has had. Very much unlike strnlen(), it has that "0
> for EFAULT" rule, and it includes the final NUL chatacter, _and_ it
> has that "retval > max" rule. They are all required, and they are all
> documented rules.
  Agreed, except for the fact that I don't think the comment explains well
that the return value larger than count+1 is possible.

								Honza
-- 
Jan Kara <jack@...e.cz>
SUSE Labs, CR
--
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/