Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755444AbcLUNI4 (ORCPT ); Wed, 21 Dec 2016 08:08:56 -0500 Received: from mail.kernel.org ([198.145.29.136]:45460 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750721AbcLUNIx (ORCPT ); Wed, 21 Dec 2016 08:08:53 -0500 MIME-Version: 1.0 In-Reply-To: <87a8bqja3i.fsf@rustcorp.com.au> References: <20161208184801.1689-1-mcgrof@kernel.org> <20161208194930.2816-1-mcgrof@kernel.org> <87k2b2kpdt.fsf@rustcorp.com.au> <20161216083123.GF13946@wotan.suse.de> <87zijvjjm1.fsf@rustcorp.com.au> <87fuljjua3.fsf@rustcorp.com.au> <87a8bqja3i.fsf@rustcorp.com.au> From: "Luis R. Rodriguez" Date: Wed, 21 Dec 2016 07:08:25 -0600 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [RFC 10/10] kmod: add a sanity check on module loading To: Rusty Russell Cc: Filipe Manana , "Paul E. McKenney" , "linux-doc@vger.kernel.org" , rgoldwyn@suse.com, hare , Jonathan Corbet , Linus Torvalds , linux-kselftest@vger.kernel.org, Andrew Morton , Dan Williams , Aaron Tomlin , rwright@hpe.com, Heinrich Schuchardt , Michal Marek , martin.wilck@suse.com, Jeff Mahoney , Ingo Molnar , Petr Mladek , Dmitry Torokhov , Guenter Roeck , "Eric W. Biederman" , shuah@kernel.org, DSterba@suse.com, Kees Cook , Josh Poimboeuf , Arnaldo Carvalho de Melo , Miroslav Benes , NeilBrown , "linux-kernel@vger.kernel.o rg" , David Miller , Jessica Yu , Subash Abhinov Kasiviswanathan , Julia Lawall Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2216 Lines: 48 On Tue, Dec 20, 2016 at 8:21 PM, Rusty Russell wrote: > "Luis R. Rodriguez" writes: >> OK -- if userspace messes up again it may be a bit hard to prove >> unless we have a validation debug thing in place, would such a thing >> in debug form be reasonable ? > > That makes perfect sense. Untested hack: > > diff --git a/fs/filesystems.c b/fs/filesystems.c > index c5618db110be..e5c90e80c7d3 100644 > --- a/fs/filesystems.c > +++ b/fs/filesystems.c > @@ -275,9 +275,10 @@ struct file_system_type *get_fs_type(const char *name) > int len = dot ? dot - name : strlen(name); > > fs = __get_fs_type(name, len); > - if (!fs && (request_module("fs-%.*s", len, name) == 0)) > + if (!fs && (request_module("fs-%.*s", len, name) == 0)) { > fs = __get_fs_type(name, len); > - > + WARN_ONCE(!fs, "request_module fs-%.*s succeeded, but still no fs?\n", len, name); > + } > if (dot && fs && !(fs->fs_flags & FS_HAS_SUBTYPE)) { > put_filesystem(fs); > fs = NULL; This is precisely a type of debug patch we had added first to verify "WTF". > Maybe a similar hack for try_then_request_module(), but many places seem > to open-code request_module() so it's not as trivial... Right, out of ~350 request_module() calls (not included try requests) only ~46 check the return value. Hence a validation check, and come to think of it, *this* was the issue that originally had me believing that in some places we might end up in a null deref --if those open coded request_module() calls assume the driver is loaded there could be many places where a NULL is inevitable. Granted, I agree they should be fixed, we could add a grammar rule to start nagging at driver developers for started, but it does beg the question also of what a tightly knit validation for modprobe might look like, and hence this patch and now the completed not-yet-posted alias work. Would it be worthy as a kconfig kmod debugging aide for now? I can follow up with a semantic patch to nag about checking the return value of request_module(), and we can have 0-day then also complain about new invalid uses. Luis