Messages in this thread | | | Date | Wed, 3 Jun 2015 15:42:49 +0200 | From | Jan Kara <> | Subject | Re: [PATCH 2/2] lib: Limit strnlen_user() return value to count + 1 |
| |
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
| |