From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755242AbcBHCWL (ORCPT ); Sun, 7 Feb 2016 21:22:11 -0500 Received: from mail.linuxfoundation.org ([140.211.169.12]:49497 "EHLO mail.linuxfoundation.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754473AbcBHCWI (ORCPT ); Sun, 7 Feb 2016 21:22:08 -0500 Date: Sun, 7 Feb 2016 18:14:12 -0800 From: Greg Kroah-Hartman To: Oleg Drokin Cc: devel@driverdev.osuosl.org, Andreas Dilger , Linux Kernel Mailing List , Lustre Development List Subject: Re: [PATCH 0/2] Lustre debugfs fixes Message-ID: <20160208021412.GB10747@kroah.com> References: <1454742111-2231460-1-git-send-email-green@linuxhacker.ru> <20160207213916.GA7016@kroah.com> <8D55EF19-8930-47C4-B7A5-27A158A1B6DB@linuxhacker.ru> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <8D55EF19-8930-47C4-B7A5-27A158A1B6DB@linuxhacker.ru> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sun, Feb 07, 2016 at 06:51:09PM -0500, Oleg Drokin wrote: > > On Feb 7, 2016, at 4:39 PM, Greg Kroah-Hartman wrote: > > > On Sat, Feb 06, 2016 at 02:01:49AM -0500, green@linuxhacker.ru wrote: > >> From: Oleg Drokin > >> > >> These two patches tie some loose ends from the Lustre debugfs conversion, > >> but while investigating them I also accumulated some questions > >> that would be good to get answers for. > >> > >> 1. Unlike procfs, debugfs does not really guard your back and if root > >> comes in and tries to write to a readonly file (or read a write-only one), > >> it's allowed (as are permission changes too) as long as the appropriate write > >> (or read) method is provided. > >> So apparently there's whole class of bugs related to this, sample > >> exhibits are in e.g. acpi_ec_add_debugfs creating a totally noop module > >> parameter to control writes that does not really prevent any writes > >> (patch submitted separately). > >> But also things like wil_debugfs_create_iomem_x32 where when called from > >> e.g. wil6210_debugfs_init_offset, some read-only attributed get a generic > >> write method that would write straight to hardware registers (who knows > >> what would happen when you write there, possibly they are readonly, but > >> you are not getting an error). > >> At first it looked like an easy way to catch this would be to just check > >> for RO/WO mode with write/read handler set, but this is thwarted by > >> the simple attribute defines that always assign read and write methods, > >> but do the check internally for the get/set method instead. > >> But also some fault injection code that sets readonly access on some files, > >> but provides a fully functional write method that works as desired. > >> > >> Would it make sense to redo the simple-attribute framework to easy such > >> cases detection (and also update writeable attributes to have permissions > >> reflecting this) and have a correspinding kernel debug compile option > >> to check for these? > > > > If a developer provides the write hooks for a debugfs file, and > > userspace changes the permissions to write to it, why would you prevent > > this? Perhaps this is what is intended. > > Well, it works differently for procfs where you cannot really change the permissions. > I understand the developer might envision permission changes (or even ignoring of > permissions by root) and there is such a code out there even. > But DEFINE_SIMPLE_ATTRIBUTE code for one always provides both read and write methods > and then internally checks if get and set are available and returns -EACCESS if not. > That's totally ok too (other than -EACCESS that I think it not permitted as an errno > from read/write with EINVAL being used for that), I am just trying to figure out if > there is a way to more automatically detect cases where the write or read access is > not desired and is left there by mistake. I don't really know, the SIMPLE_ATTRIBUTE() stuff was just a way to try to reduce a ton of boiler-plate code that was all over the place. I doubt anyone thought of this when it was added (I know I didn't.) > > Remember, debugfs is only for debugging stuff, never rely on it for > > actual device/system use. > > Yes, I understand that. But debugfs is on by default pretty much anywhere But it's root-only, thankfully. If only ftrace wouldn't use it, distros could turn it off entirely :( > and if there's a bug that lets you write to some hardware register you > were supposed to only read and get some elevated privileges, or you > read some write-only file and crash the kernel - that's bad too. I agree, again, which is why it's mounted root-only. Also, bad things happen when you remove debugfs files while someone has a file open :) thanks, greg k-h