Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Fri, 7 Feb 2020 16:44:49 -0800
From:   Kees Cook <keescook@chromium.org>
To:     Steven Rostedt <rostedt@goodmis.org>
Cc:     Masami Hiramatsu <mhiramat@kernel.org>,
        Ingo Molnar <mingo@redhat.com>,
        Frank Rowand <frowand.list@gmail.com>,
        Randy Dunlap <rdunlap@infradead.org>,
        Namhyung Kim <namhyung@kernel.org>,
        Tim Bird <Tim.Bird@sony.com>, Jiri Olsa <jolsa@redhat.com>,
        Arnaldo Carvalho de Melo <acme@kernel.org>,
        Tom Zanussi <tom.zanussi@linux.intel.com>,
        Rob Herring <robh+dt@kernel.org>,
        Andrew Morton <akpm@linux-foundation.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Alexey Dobriyan <adobriyan@gmail.com>,
        Jonathan Corbet <corbet@lwn.net>,
        Linus Torvalds <torvalds@linux-foundation.org>,
        linux-doc@vger.kernel.org, linux-fsdevel@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v6 08/22] bootconfig: init: Allow admin to use bootconfig
 for init command line
Message-ID: <202002071640.49BFDA2D1A@keescook>
References: <157867220019.17873.13377985653744804396.stgit@devnote2>
 <157867229521.17873.654222294326542349.stgit@devnote2>
 <202002070954.C18E7F58B@keescook>
 <20200207144603.30688b94@oasis.local.home>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200207144603.30688b94@oasis.local.home>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Fri, Feb 07, 2020 at 02:46:03PM -0500, Steven Rostedt wrote:
> On Fri, 7 Feb 2020 10:03:16 -0800
> Kees Cook <keescook@chromium.org> wrote:
> > > +		len = strlen(saved_command_line);
> > > +		if (!strstr(boot_command_line, " -- ")) {
> > > +			strcpy(saved_command_line + len, " -- ");
> > > +			len += 4;
> > > +		} else
> > > +			saved_command_line[len++] = ' ';
> > > +
> > > +		strcpy(saved_command_line + len, extra_init_args);
> > > +	}  
> > 
> > This isn't safe because it will destroy any argument with " -- " in
> > quotes and anything after it. For example, booting with:
> > 
> > thing=on acpi_osi="! -- " other=setting
> > 
> > will wreck acpi_osi's value and potentially overwrite "other=settings",
> > etc.
> > 
> > (Yes, this seems very unlikely, but you can't treat " -- " as special,
> > the command line string must be correct parsed for double quotes, as
> > parse_args() does.)
> > 
> 
> This is not the args you are looking for. ;-)
> 
> There is a slight bug, but not as bad as you may think it is.
> bootconfig (when added to the command line) will look for a json like
> file appended to the initrd, and it will parse that. That's what all the
> xbc_*() functions do (extended boot commandline). If one of the options
> in that json like file is "init", then it will create the
> extra_init_args, which will make ilen greater than zero.
> 
> The above if statement looks for that ' -- ', and if it doesn't find it
> (strcmp() returns NULL when not found) it will than append " -- " to
> the boot_command_line. If it is found, then the " -- " is not added. In
> either case, the init args found in the json like file in the initrd is
> appended to the saved_command_line.
> 
> I did say there's a slight bug here. If you have your condition, and
> you add init arguments to that json file, it wont properly add the " --
> ", and the init arguments in that file will be ignored.

Ah, right, it's even more slight, sorry, I had the strstr() in my head
still. So, yes, with an "init" section and a very goofy " -- " present
in a kernel bootparam string value, the appended init args will be
parsed as kernel options.

-- 
Kees Cook