From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.8 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 6567EC3F2CD for ; Fri, 28 Feb 2020 21:31:28 +0000 (UTC) Received: from mother.openwall.net (mother.openwall.net [195.42.179.200]) by mail.kernel.org (Postfix) with SMTP id 94BB7246AE for ; Fri, 28 Feb 2020 21:31:27 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 94BB7246AE Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=xmission.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=kernel-hardening-return-18016-kernel-hardening=archiver.kernel.org@lists.openwall.com Received: (qmail 11664 invoked by uid 550); 28 Feb 2020 21:31:20 -0000 Mailing-List: contact kernel-hardening-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Received: (qmail 11639 invoked from network); 28 Feb 2020 21:31:19 -0000 From: ebiederm@xmission.com (Eric W. Biederman) To: Christian Brauner Cc: linux-kernel@vger.kernel.org, Al Viro , Kernel Hardening , Linux API , Linux FS Devel , Linux Security Module , Akinobu Mita , Alexey Dobriyan , Andrew Morton , Andy Lutomirski , Daniel Micay , Djalal Harouni , "Dmitry V . Levin" , Greg Kroah-Hartman , Ingo Molnar , "J . Bruce Fields" , Jeff Layton , Jonathan Corbet , Kees Cook , Oleg Nesterov , Alexey Gladkov , Linus Torvalds , Jeff Dike , Richard Weinberger , Anton Ivanov References: <20200212203833.GQ23230@ZenIV.linux.org.uk> <20200212204124.GR23230@ZenIV.linux.org.uk> <87lfp7h422.fsf@x220.int.ebiederm.org> <87pnejf6fz.fsf@x220.int.ebiederm.org> <871rqpaswu.fsf_-_@x220.int.ebiederm.org> <871rqk2brn.fsf_-_@x220.int.ebiederm.org> <878skmsbyy.fsf_-_@x220.int.ebiederm.org> <87wo86qxcs.fsf_-_@x220.int.ebiederm.org> <20200228203058.jcnqeyvmqhfslcym@wittgenstein> Date: Fri, 28 Feb 2020 15:28:54 -0600 In-Reply-To: <20200228203058.jcnqeyvmqhfslcym@wittgenstein> (Christian Brauner's message of "Fri, 28 Feb 2020 21:30:58 +0100") Message-ID: <87zhd2pfjd.fsf@x220.int.ebiederm.org> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-XM-SPF: eid=1j7nDp-0005Of-9H;;;mid=<87zhd2pfjd.fsf@x220.int.ebiederm.org>;;;hst=in02.mta.xmission.com;;;ip=68.227.160.95;;;frm=ebiederm@xmission.com;;;spf=neutral X-XM-AID: U2FsdGVkX18C5s6UU8g6m3GJYbDK37dFKpX40ZtE5BE= X-SA-Exim-Connect-IP: 68.227.160.95 X-SA-Exim-Mail-From: ebiederm@xmission.com Subject: Re: [PATCH 2/3] uml: Create a private mount of proc for mconsole X-SA-Exim-Version: 4.2.1 (built Thu, 05 May 2016 13:38:54 -0600) X-SA-Exim-Scanned: Yes (on in02.mta.xmission.com) Christian Brauner writes: > On Fri, Feb 28, 2020 at 02:18:43PM -0600, Eric W. Biederman wrote: >> >> The mconsole code only ever accesses proc for the initial pid >> namespace. Instead of depending upon the proc_mnt which is >> for proc_flush_task have uml create it's own mount of proc >> instead. >> >> This allows proc_flush_task to evolve and remove the >> need for having a proc_mnt to do it's job. >> >> Cc: Jeff Dike >> Cc: Richard Weinberger >> Cc: Anton Ivanov >> Signed-off-by: Eric W. Biederman >> --- >> arch/um/drivers/mconsole_kern.c | 28 +++++++++++++++++++++++++++- >> 1 file changed, 27 insertions(+), 1 deletion(-) >> >> diff --git a/arch/um/drivers/mconsole_kern.c b/arch/um/drivers/mconsole_kern.c >> index e8f5c81c2c6c..30575bd92975 100644 >> --- a/arch/um/drivers/mconsole_kern.c >> +++ b/arch/um/drivers/mconsole_kern.c >> @@ -36,6 +36,8 @@ >> #include "mconsole_kern.h" >> #include >> >> +static struct vfsmount *proc_mnt = NULL; >> + >> static int do_unlink_socket(struct notifier_block *notifier, >> unsigned long what, void *data) >> { >> @@ -123,7 +125,7 @@ void mconsole_log(struct mc_request *req) >> >> void mconsole_proc(struct mc_request *req) >> { >> - struct vfsmount *mnt = init_pid_ns.proc_mnt; >> + struct vfsmount *mnt = proc_mnt; >> char *buf; >> int len; >> struct file *file; >> @@ -134,6 +136,10 @@ void mconsole_proc(struct mc_request *req) >> ptr += strlen("proc"); >> ptr = skip_spaces(ptr); >> >> + if (!mnt) { >> + mconsole_reply(req, "Proc not available", 1, 0); >> + goto out; >> + } >> file = file_open_root(mnt->mnt_root, mnt, ptr, O_RDONLY, 0); >> if (IS_ERR(file)) { >> mconsole_reply(req, "Failed to open file", 1, 0); >> @@ -683,6 +689,24 @@ void mconsole_stack(struct mc_request *req) >> with_console(req, stack_proc, to); >> } >> >> +static int __init mount_proc(void) >> +{ >> + struct file_system_type *proc_fs_type; >> + struct vfsmount *mnt; >> + >> + proc_fs_type = get_fs_type("proc"); >> + if (!proc_fs_type) >> + return -ENODEV; >> + >> + mnt = kern_mount(proc_fs_type); >> + put_filesystem(proc_fs_type); >> + if (IS_ERR(mnt)) >> + return PTR_ERR(mnt); >> + >> + proc_mnt = mnt; >> + return 0; >> +} >> + >> /* >> * Changed by mconsole_setup, which is __setup, and called before SMP is >> * active. >> @@ -696,6 +720,8 @@ static int __init mconsole_init(void) >> int err; >> char file[UNIX_PATH_MAX]; >> >> + mount_proc(); > > Hm, either check the return value or make the mount_proc() void? > Probably worth logging something but moving on without proc. I modified mconsole_proc (the only place that cares to see if it has a valid proc_mnt). So the code already does the moving on without mounting proc and continues to work. Further the code logs something when it tries to use the mount of proc and proc is not available. I think this can happen if someone is strange enough to compile the kernel without proc. So at least in some scenarios I believe it is expected that it will fail. So while I think it is good form to generate good error codes in the incredibly unlikely case that proc_mount() fails during boot I don't see the point of doing anything with them. > I guess this is user visible in some scenarios but the patch series > seems worth it! What scenarios do you think this would be user visible? The set of calls to mount proc are slightly different, but the options to proc when mounting (none) remain the same. For the series as a whole the only place where it should be user visible is when the proc mount options start getting honored. AKA when hidepid=N starts working as designed again. Eric