From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754426AbbFCNm6 (ORCPT ); Wed, 3 Jun 2015 09:42:58 -0400 Received: from cantor2.suse.de ([195.135.220.15]:50765 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752652AbbFCNmv (ORCPT ); Wed, 3 Jun 2015 09:42:51 -0400 Date: Wed, 3 Jun 2015 15:42:49 +0200 From: Jan Kara To: Linus Torvalds Cc: Jan Kara , Andrew Morton , LKML Subject: Re: [PATCH 2/2] lib: Limit strnlen_user() return value to count + 1 Message-ID: <20150603134249.GA2270@quack.suse.cz> References: <1433257829-1743-1-git-send-email-jack@suse.cz> <1433257829-1743-2-git-send-email-jack@suse.cz> <20150603092129.GE13054@quack.suse.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed 03-06-15 06:21:45, Linus Torvalds wrote: > On Wed, Jun 3, 2015 at 2:21 AM, Jan Kara 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 SUSE Labs, CR