[PATCH 1/2] lib: Fix strnlen_user() to not touch memory after specified maximum

[PATCH 1/2] lib: Fix strnlen_user() to not touch memory after specified maximum

[Jan Kara]

If the specified maximum length of the string is a multiple of unsigned
long, we would load one long behind the specified maximum. If that
happens to be in a next page, we can hit a page fault although we were
not expected to.

Fix the off-by-one bug in the test whether we are at the end of the
specified range.

CC: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Jan Kara <jack@suse.cz>
---
 lib/strnlen_user.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/strnlen_user.c b/lib/strnlen_user.c
index a28df5206d95..fd03ae980013 100644
--- a/lib/strnlen_user.c
+++ b/lib/strnlen_user.c
@@ -57,7 +57,7 @@ static inline long do_strnlen_user(const char __user *src, unsigned long count,
 			return res + find_zero(data) + 1 - align;
 		}
 		res += sizeof(unsigned long);
-		if (unlikely(max < sizeof(unsigned long)))
+		if (unlikely(max <= sizeof(unsigned long)))
 			break;
 		max -= sizeof(unsigned long);
 		if (unlikely(__get_user(c,(unsigned long __user *)(src+res))))
-- 
2.1.4

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

[Jan Kara]

Currently strnlen_user() can return numbers between 0 and
count + sizeof(unsigned long) - 1. Currently, no in tree users seem to
care but I have found out of tree users which were broken by this. They
wanted to truncate the string if it was too long to fit into a buffer
and didn't count with the fact that strnlen_user() can return more.
So make the function harder to use wrong and return count + 1 max.

CC: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Jan Kara <jack@suse.cz>
---
 lib/strnlen_user.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/lib/strnlen_user.c b/lib/strnlen_user.c
index fd03ae980013..2e47f9e16a79 100644
--- a/lib/strnlen_user.c
+++ b/lib/strnlen_user.c
@@ -54,7 +54,10 @@ static inline long do_strnlen_user(const char __user *src, unsigned long count,
 		if (has_zero(c, &data, &constants)) {
 			data = prep_zero_mask(c, data, &constants);
 			data = create_zero_mask(data);
-			return res + find_zero(data) + 1 - align;
+			res = res + find_zero(data) + 1 - align;
+			if (res > count)
+				return count + 1;
+			return res;
 		}
 		res += sizeof(unsigned long);
 		if (unlikely(max <= sizeof(unsigned long)))
-- 
2.1.4

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

[Linus Torvalds]

On Tue, Jun 2, 2015 at 8:10 AM, Jan Kara <jack@suse.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.

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.

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.

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.

               Linus

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

[Jan Kara]

On Tue 02-06-15 10:08:11, Linus Torvalds wrote:
> On Tue, Jun 2, 2015 at 8:10 AM, Jan Kara <jack@suse.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@suse.cz>
SUSE Labs, CR

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@suse.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

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

[Jan Kara]

On Wed 03-06-15 06:21:45, Linus Torvalds wrote:
> On Wed, Jun 3, 2015 at 2:21 AM, Jan Kara <jack@suse.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
  OK, I'll send a fix.

> > 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.
  Yup, that's a good point.

> [ 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.
  Fair enough.

> I'd almost be inclined to unexport it. From a quick look, we don't
> have any module users.
  Audit code (kernel/auditsc.c) uses it for arguments of executables so
that looks like a valid use from a module...

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: [PATCH 1/2] lib: Fix strnlen_user() to not touch memory after specified maximum

[Linus Torvalds]

On Tue, Jun 2, 2015 at 8:10 AM, Jan Kara <jack@suse.cz> wrote:
>
> Fix the off-by-one bug in the test whether we are at the end of the
> specified range.

Ack. I'll apply it with an added comment.

             Linus