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

[Jan Kara <jack@suse.cz>]

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