On Wed 03-06-15 06:21:45, Linus Torvalds wrote:
> On Wed, Jun 3, 2015 at 2:21 AM, Jan Kara <[email protected]> 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 <[email protected]>
SUSE Labs, CR
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
