linux-kernel - Re: [PATCH] kernel/sysctl.c: If "count" including the terminating byte '\0' the write system call should retrun success.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 8 Sep 2015 12:36:16 -0400
From:	Steven Rostedt <rostedt@...dmis.org>
To:	ebiederm@...ssion.com (Eric W. Biederman)
Cc:	Sean Fu <fxinrong@...il.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Austin S Hemmelgarn <ahferroin7@...il.com>,
	Andrey Ryabinin <ryabinin.a.a@...il.com>,
	Ulrich Obergfell <uobergfe@...hat.com>,
	Prarit Bhargava <prarit@...hat.com>,
	Eric B Munson <emunson@...mai.com>,
	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>,
	Johannes Weiner <hannes@...xchg.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	Don Zickus <dzickus@...hat.com>,
	Heinrich Schuchardt <xypron.glpk@....de>,
	David Rientjes <rientjes@...gle.com>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] kernel/sysctl.c: If "count" including the terminating
 byte '\0' the write system call should retrun success.

On Tue, 08 Sep 2015 11:19:14 -0500
ebiederm@...ssion.com (Eric W. Biederman) wrote:


> This patch does not implement the old behavior.
> 
> The old code does use '\0' as a buffer terminator, and because it does
> not check things closely I can see how it could accept a '\0' from
> userspace and treat that as an early buffer terminator.
> 
> The patch treats '\0' as a number separator and allows things that have
> never been allowed before and quite frankly is very scary as it just
> invites bugs.
> 
> So I do not think we should merge the given patch.  It is just wrong.
> One that simply truncates the input buffer at the first '\0' character I
> think we can consider, although I am not a fan.

I agree, and was thinking this patch did that, but I didn't look close
enough (why I never gave a Reviewed-by to it either).


> 
> Steve as far as what I think would break.  I don't think the current
> behavior should have broken anything and apparently it did.  I don't see
> what a change that simply truncates the buffer at the first embedded
> '\0' would break, but I don't know how to test that there isn't anything
> that it will.  We are way past the point of reasonable expectations
> being able to guide us.  4 years should have been more than enough soak
> time to have been able to say that the change was good, but apparently
> it was not.

Well, to be fair, a lot of people (distros, etc) do not use the most
recent kernels. 4 years may be the first time a tool touches a kernel.

> 
> My gut feel says that if we are going to change this, at this late date,
> we find the one specific proc file that matters and change it just for
> that one proc file, and in that change we treat '\0' as a terminator not
> as a separator.  I never did see in the conversation which proc file it
> is that actually matters.  The principle is that the more precise and
> the more localized such a change is the less chance it has of causing a
> regression of something else, and the greater the chance we can look at
> a specific issue.

Sounds like a reasonable compromise. Sean, can you make a patch that
only affects the one proc file (comment it well in the code), and have
it accept nothing past the '\0'. Even if someone passed in "1 \0 2", it
would only see "1 "

-- Steve
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/