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@xxxxxxxxxxxxxx wrote: > >> From: Oleg Drokin <green@xxxxxxxxxxxxxx> > >> > >> 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 _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
