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]

Message-ID: <87mvwql15x.fsf@x220.int.ebiederm.org>
Date:	Sun, 13 Sep 2015 11:44:10 -0500
From:	ebiederm@...ssion.com (Eric W. Biederman)
To:	Sean Fu <fxinrong@...il.com>
Cc:	Steven Rostedt <rostedt@...dmis.org>,
	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.

Sean Fu <fxinrong@...il.com> writes:

> On Sat, Sep 12, 2015 at 1:01 AM, Eric W. Biederman
> <ebiederm@...ssion.com> wrote:
>> Sean Fu <fxinrong@...il.com> writes:
>>
>>>> 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 "
>>> The current code uses uniform handler (e.g. "proc_dointvec") for all
>>> same type proc file.
>>> So all integer type proc file are affected.
>>
>> No.  I do not believe the proprietary binary application you are dealing
>> with writes to all proc files that use the proc_dointvec handler.
> I means all ctl_table whose .proc_handler is "proc_dointvec" are
> affected.

I mean this only deserves consideration because this is a regression
report.

I mean by limiting this to only the proc files that are written to by
the weird program that broke we can minimize the chances that anything
else will break.

I do not believe that the weird program that broke writes to every proc
file.  Certainly I have not heard that asserted.

>>> In fact, The behavior of all integer type proc file should be changed.
>>
>> Not at all.  The only files that we can possibly justify changing today
>> are the files where an actual regression is being observed.
>>
>> Because quite frankly 5 years is way too long to wait to report a
>> regression.  By and large software is reasonable and treats proc
>> files as text files where '\0' is an invalid character.
> 5 years is not enough long for distros, specially enterprise distros.
> The most of HuaWei machines run our SLES10sp3(2.6.16, SUSE LINUX
> ENTERPRISE SERVER).
> They use one enterprise version for 5+ years usually.

If you want to play by enterprise kernel rules please talk to your
enterprise kernel support people.

>> Accepting a '\0' is not at all reasonable for a text interface.  The
>> application that does it is buggy.
> It is hard to comprehend that the current kernel can accept  two bytes
> "1 ", "1\t", "1\n" except "1\0".

'\0' is not and has never been valid in a text file.
proc files are a text interface.
Expecting '\0' to be accepted is very strange, and apparently there is
only one program in existence that does.

That a trailing '\0' was ever accepted was due to a bug in the code.

Accepting '\0' in general in a text interface is a very dangerous and
buggy pattern so it must be done very carefully or else other
regressions or bugs could be easily introduced.

That no one has complained about this in the 5 years since the change happened
strongly indicates this no one else cares.

A very targeted very narrow regression fix that only handles a trailing
'\0' and that only changes the behavior of proc files that matter is
reasonable.

Or do you volunteer to go out and test every program that has been
written or updated to write to proc in the last 5 years (since the
behavior changed) and verify that none of them in no circumstances
depend upon failing if an trailing '\0' is included?   If you can audit
all of the code written in the last 5 years and verify that the change
will not introduce problems for any other user space program we can talk
about changing all of the proc files that use proc_dointvec.

Eric
--
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/