| Message ID | 152346403637.4030.15247096217928429102.stgit@warthog.procyon.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 04/11/2018 09:27 AM, David Howells wrote: > Signed-off-by: David Howells <dhowells@redhat.com> > cc: Andy Shevchenko <andy.shevchenko@gmail.com> > cc: acpi4asus-user@lists.sourceforge.net > cc: platform-driver-x86@vger.kernel.org > cc: Matthew Garrett <mjg59@srcf.ucam.org> > cc: Thomas Gleixner <tglx@linutronix.de> > --- meta-comment: I have been dinged for not spelling "cc:" as "Cc:". I really think that either way should be acceptable.
David Howells <dhowells@redhat.com> writes: > Disallow opening of debugfs files that might be used to muck around when > the kernel is locked down as various drivers give raw access to hardware > through debugfs. Given the effort of auditing all 2000 or so files and > manually fixing each one as necessary, I've chosen to apply a heuristic > instead. The following changes are made: > > (1) chmod and chown are disallowed on debugfs objects (though the root dir > can be modified by mount and remount, but I'm not worried about that). > > (2) When the kernel is locked down, only files with the following criteria > are permitted to be opened: > > - The file must have mode 00444 > - The file must not have ioctl methods > - The file must not have mmap > > (3) When the kernel is locked down, files may only be opened for reading. > > Normal device interaction should be done through configfs, sysfs or a > miscdev, not debugfs. > Note that this makes it unnecessary to specifically lock down show_dsts(), > show_devs() and show_call() in the asus-wmi driver. > > I would actually prefer to lock down all files by default and have the > the files unlocked by the creator. This is tricky to manage correctly, > though, as there are 19 creation functions and ~1600 call sites (some of > them in loops scanning tables). Why is mounting debugfs allowed at all? Last I checked (it has been a while) the code quality of debugfs was fine for debugging but debugfs was not safe to mount on a production system. Maybe the code quality is better now but for a filesystem that is not supposed to be needed for developers letting us mount debugfs seems odd. Eric -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Apr 11, 2018 at 05:27:16PM +0100, David Howells wrote: > Disallow opening of debugfs files that might be used to muck around when > the kernel is locked down as various drivers give raw access to hardware > through debugfs. Given the effort of auditing all 2000 or so files and > manually fixing each one as necessary, I've chosen to apply a heuristic > instead. The following changes are made: > > (1) chmod and chown are disallowed on debugfs objects (though the root dir > can be modified by mount and remount, but I'm not worried about that). > > (2) When the kernel is locked down, only files with the following criteria > are permitted to be opened: > > - The file must have mode 00444 > - The file must not have ioctl methods > - The file must not have mmap > > (3) When the kernel is locked down, files may only be opened for reading. > > Normal device interaction should be done through configfs, sysfs or a > miscdev, not debugfs. > > Note that this makes it unnecessary to specifically lock down show_dsts(), > show_devs() and show_call() in the asus-wmi driver. > > I would actually prefer to lock down all files by default and have the > the files unlocked by the creator. This is tricky to manage correctly, > though, as there are 19 creation functions and ~1600 call sites (some of > them in loops scanning tables). Why not just disable debugfs entirely? This half-hearted way to sorta lock it down is odd, it is meant to not be there at all, nothing in your normal system should ever depend on it. So again just don't allow it to be mounted at all, much simpler and more obvious as to what is going on. thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Eric W. Biederman <ebiederm@xmission.com> wrote: > Why is mounting debugfs allowed at all? Last I checked (it has been a while) > the code quality of debugfs was fine for debugging but debugfs was not > safe to mount on a production system. > > Maybe the code quality is better now but for a filesystem that is > not supposed to be needed for developers letting us mount debugfs > seems odd. I agree. But debugfs has been abused and it seems that there are some things that use it as an interface between a kernel driver and the userspace side. David -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Greg KH <greg@kroah.com> wrote: > Why not just disable debugfs entirely? This half-hearted way to sorta > lock it down is odd, it is meant to not be there at all, nothing in your > normal system should ever depend on it. > > So again just don't allow it to be mounted at all, much simpler and more > obvious as to what is going on. Yeah, I agree - and then I got complaints because it seems that it's been abused to allow drivers and userspace components to communicate. David -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Apr 11, 2018 at 09:09:16PM +0100, David Howells wrote: > Greg KH <greg@kroah.com> wrote: > > > Why not just disable debugfs entirely? This half-hearted way to sorta > > lock it down is odd, it is meant to not be there at all, nothing in your > > normal system should ever depend on it. > > > > So again just don't allow it to be mounted at all, much simpler and more > > obvious as to what is going on. > > Yeah, I agree - and then I got complaints because it seems that it's been > abused to allow drivers and userspace components to communicate. With in-kernel code? Please let me know and I'll go fix it up to not allow that, as that is not ok. I do know of some bad examples of out-of-tree code abusing debugfs to do crazy things (battery level monitoring?), but that's their own fault... debugfs is for DEBUGGING! For anything you all feel should be "secure", then just disable it entirely. thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Apr 11, 2018 at 1:33 PM, Greg KH <greg@kroah.com> wrote: > On Wed, Apr 11, 2018 at 09:09:16PM +0100, David Howells wrote: >> Greg KH <greg@kroah.com> wrote: >> >> > Why not just disable debugfs entirely? This half-hearted way to sorta >> > lock it down is odd, it is meant to not be there at all, nothing in your >> > normal system should ever depend on it. >> > >> > So again just don't allow it to be mounted at all, much simpler and more >> > obvious as to what is going on. >> >> Yeah, I agree - and then I got complaints because it seems that it's been >> abused to allow drivers and userspace components to communicate. > > With in-kernel code? Please let me know and I'll go fix it up to not > allow that, as that is not ok. > > I do know of some bad examples of out-of-tree code abusing debugfs to do > crazy things (battery level monitoring?), but that's their own fault... > > debugfs is for DEBUGGING! For anything you all feel should be "secure", > then just disable it entirely. > Debugfs is very, very useful for, ahem, debugging. I really think this is an example of why we should split lockdown into the read and write varieties and allow mounting and reading debugfs when only write is locked down. -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Apr 11, 2018 at 07:54:12PM -0700, Andy Lutomirski wrote: > On Wed, Apr 11, 2018 at 1:33 PM, Greg KH <greg@kroah.com> wrote: > > On Wed, Apr 11, 2018 at 09:09:16PM +0100, David Howells wrote: > >> Greg KH <greg@kroah.com> wrote: > >> > >> > Why not just disable debugfs entirely? This half-hearted way to sorta > >> > lock it down is odd, it is meant to not be there at all, nothing in your > >> > normal system should ever depend on it. > >> > > >> > So again just don't allow it to be mounted at all, much simpler and more > >> > obvious as to what is going on. > >> > >> Yeah, I agree - and then I got complaints because it seems that it's been > >> abused to allow drivers and userspace components to communicate. > > > > With in-kernel code? Please let me know and I'll go fix it up to not > > allow that, as that is not ok. > > > > I do know of some bad examples of out-of-tree code abusing debugfs to do > > crazy things (battery level monitoring?), but that's their own fault... > > > > debugfs is for DEBUGGING! For anything you all feel should be "secure", > > then just disable it entirely. > > > > Debugfs is very, very useful for, ahem, debugging. I really think > this is an example of why we should split lockdown into the read and > write varieties and allow mounting and reading debugfs when only write > is locked down. Ok, but be sure that there are no "secrets" in those debugging files if you really buy into the whole "lock down" mess... Really, it's easier to just disable the whole thing. greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Apr 12, 2018 at 1:23 AM, Greg KH <greg@kroah.com> wrote: > On Wed, Apr 11, 2018 at 07:54:12PM -0700, Andy Lutomirski wrote: >> On Wed, Apr 11, 2018 at 1:33 PM, Greg KH <greg@kroah.com> wrote: >> > On Wed, Apr 11, 2018 at 09:09:16PM +0100, David Howells wrote: >> >> Greg KH <greg@kroah.com> wrote: >> >> >> >> > Why not just disable debugfs entirely? This half-hearted way to sorta >> >> > lock it down is odd, it is meant to not be there at all, nothing in your >> >> > normal system should ever depend on it. >> >> > >> >> > So again just don't allow it to be mounted at all, much simpler and more >> >> > obvious as to what is going on. >> >> >> >> Yeah, I agree - and then I got complaints because it seems that it's been >> >> abused to allow drivers and userspace components to communicate. >> > >> > With in-kernel code? Please let me know and I'll go fix it up to not >> > allow that, as that is not ok. >> > >> > I do know of some bad examples of out-of-tree code abusing debugfs to do >> > crazy things (battery level monitoring?), but that's their own fault... >> > >> > debugfs is for DEBUGGING! For anything you all feel should be "secure", >> > then just disable it entirely. >> > >> >> Debugfs is very, very useful for, ahem, debugging. I really think >> this is an example of why we should split lockdown into the read and >> write varieties and allow mounting and reading debugfs when only write >> is locked down. > > Ok, but be sure that there are no "secrets" in those debugging files if > you really buy into the whole "lock down" mess... > > Really, it's easier to just disable the whole thing. > I mostly agree with your sentiment. I'm saying that, for most uses, I *don't* buy into the idea that a normal secure-boot-supporting distro should block debugfs. I sometimes like to ask people who report problems to send me the contents of such-and-such file in debugfs, and I think it should keep working. Blocking write access to debugfs is mostly sensible for a lockdown system, but blocking read only makes sense if you're worried about straight-up bugs or if you think that debugfs contains protection-worthy secrets. What I want to see is: lockdown=protect_integrity: debugfs is read-only, bpf and perf are unrestricted, iopl and ioperm are disabled, etc. lockdown=protect_integrity_and_secrecy: debugfs is gone, bpf and perf are restricted, plus all the restrictions from lockdown=protect_integrity Distros should strongly prefer lockdown=protect_integrity (or lockdown=off) by default. lockdown=protect_integrity_and_secrecy is for custom setups, embedded applications, etc. --Andy -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed 2018-04-11 17:27:16, David Howells wrote: > Disallow opening of debugfs files that might be used to muck around when > the kernel is locked down as various drivers give raw access to hardware > through debugfs. Given the effort of auditing all 2000 or so files and > manually fixing each one as necessary, I've chosen to apply a heuristic > instead. The following changes are made: > > (1) chmod and chown are disallowed on debugfs objects (though the root dir > can be modified by mount and remount, but I'm not worried about that). This has nothing to do with the lockdown goals, right? I find chown of such files quite nice, to allow debugging without doing sudo all the time. > (2) When the kernel is locked down, only files with the following criteria > are permitted to be opened: > > - The file must have mode 00444 > - The file must not have ioctl methods > - The file must not have mmap Dunno. Would not it be nicer to go through the debugfs files and split them into safe/unsafe varieties? Pavel
Pavel Machek <pavel@ucw.cz> wrote: > > (1) chmod and chown are disallowed on debugfs objects (though the root dir > > can be modified by mount and remount, but I'm not worried about that). > > This has nothing to do with the lockdown goals, right? I find chown of > such files quite nice, to allow debugging without doing sudo all the time. It allows someone to give everyone access to files that should perhaps only be accessible by root. Besides, if you disable lockdown then you can do this if you want. > > (2) When the kernel is locked down, only files with the following criteria > > are permitted to be opened: > > > > - The file must have mode 00444 > > - The file must not have ioctl methods > > - The file must not have mmap > > Dunno. Would not it be nicer to go through the debugfs files and split > them into safe/unsafe varieties? Whilst that is a laudable idea, there are at least a couple of thousand files to analyse, some of them doing weird stuff in drivers that aren't easy to understand, and some of them with lists of files, some of which might be safe and some of which aren't safe. Even some reads that look like they ought to be safe have side effects (such as read-and-reset counters). Besides, define 'safe'. Is reading a particular reg on a device and returning the value through a debugfs read safe, for example? What about files where reading is harmless, but writing needs to be disallowed? I did try initially passing in a flag to say whether something was safe or not, abusing an inode flag to store it since debugfs uses a plain inode struct, but then I realised just how many ways there are to create debugfs files and it started to get out of hand. My initial idea also included locking everything down by default and marking things unlocked on an as-needed basis. If you have the time to audit all these files, then that would be great. I lean more towards the lock debugfs down entirely side. David -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu 2018-04-19 15:35:47, David Howells wrote: > Pavel Machek <pavel@ucw.cz> wrote: > > > > (1) chmod and chown are disallowed on debugfs objects (though the root dir > > > can be modified by mount and remount, but I'm not worried about that). > > > > This has nothing to do with the lockdown goals, right? I find chown of > > such files quite nice, to allow debugging without doing sudo all the time. > > It allows someone to give everyone access to files that should perhaps only be > accessible by root. Besides, if you disable lockdown then you can do this if > you want. As I said this has nothing to do with lockdown, so does not belong in this series. (And besides, it is bad idea.) Pavel
diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c index 1f99678ff5d3..51cb894c21f2 100644 --- a/fs/debugfs/file.c +++ b/fs/debugfs/file.c @@ -136,6 +136,25 @@ void debugfs_file_put(struct dentry *dentry) } EXPORT_SYMBOL_GPL(debugfs_file_put); +/* + * Only permit access to world-readable files when the kernel is locked down. + * We also need to exclude any file that has ways to write or alter it as root + * can bypass the permissions check. + */ +static bool debugfs_is_locked_down(struct inode *inode, + struct file *filp, + const struct file_operations *real_fops) +{ + if ((inode->i_mode & 07777) == 0444 && + !(filp->f_mode & FMODE_WRITE) && + !real_fops->unlocked_ioctl && + !real_fops->compat_ioctl && + !real_fops->mmap) + return false; + + return kernel_is_locked_down("debugfs"); +} + static int open_proxy_open(struct inode *inode, struct file *filp) { struct dentry *dentry = F_DENTRY(filp); @@ -147,6 +166,11 @@ static int open_proxy_open(struct inode *inode, struct file *filp) return r == -EIO ? -ENOENT : r; real_fops = debugfs_real_fops(filp); + + r = -EPERM; + if (debugfs_is_locked_down(inode, filp, real_fops)) + goto out; + real_fops = fops_get(real_fops); if (!real_fops) { /* Huh? Module did not clean up after itself at exit? */ @@ -272,6 +296,10 @@ static int full_proxy_open(struct inode *inode, struct file *filp) return r == -EIO ? -ENOENT : r; real_fops = debugfs_real_fops(filp); + r = -EPERM; + if (debugfs_is_locked_down(inode, filp, real_fops)) + goto out; + real_fops = fops_get(real_fops); if (!real_fops) { /* Huh? Module did not cleanup after itself at exit? */ diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c index 13b01351dd1c..4daec17b8215 100644 --- a/fs/debugfs/inode.c +++ b/fs/debugfs/inode.c @@ -32,6 +32,31 @@ static struct vfsmount *debugfs_mount; static int debugfs_mount_count; static bool debugfs_registered; +/* + * Don't allow access attributes to be changed whilst the kernel is locked down + * so that we can use the file mode as part of a heuristic to determine whether + * to lock down individual files. + */ +static int debugfs_setattr(struct dentry *dentry, struct iattr *ia) +{ + if ((ia->ia_valid & (ATTR_MODE | ATTR_UID | ATTR_GID)) && + kernel_is_locked_down("debugfs")) + return -EPERM; + return simple_setattr(dentry, ia); +} + +static const struct inode_operations debugfs_file_inode_operations = { + .setattr = debugfs_setattr, +}; +static const struct inode_operations debugfs_dir_inode_operations = { + .lookup = simple_lookup, + .setattr = debugfs_setattr, +}; +static const struct inode_operations debugfs_symlink_inode_operations = { + .get_link = simple_get_link, + .setattr = debugfs_setattr, +}; + static struct inode *debugfs_get_inode(struct super_block *sb) { struct inode *inode = new_inode(sb); @@ -356,6 +381,7 @@ static struct dentry *__debugfs_create_file(const char *name, umode_t mode, inode->i_mode = mode; inode->i_private = data; + inode->i_op = &debugfs_file_inode_operations; inode->i_fop = proxy_fops; dentry->d_fsdata = (void *)((unsigned long)real_fops | DEBUGFS_FSDATA_IS_REAL_FOPS_BIT); @@ -513,7 +539,7 @@ struct dentry *debugfs_create_dir(const char *name, struct dentry *parent) return failed_creating(dentry); inode->i_mode = S_IFDIR | S_IRWXU | S_IRUGO | S_IXUGO; - inode->i_op = &simple_dir_inode_operations; + inode->i_op = &debugfs_dir_inode_operations; inode->i_fop = &simple_dir_operations; /* directory inodes start off with i_nlink == 2 (for "." entry) */ @@ -608,7 +634,7 @@ struct dentry *debugfs_create_symlink(const char *name, struct dentry *parent, return failed_creating(dentry); } inode->i_mode = S_IFLNK | S_IRWXUGO; - inode->i_op = &simple_symlink_inode_operations; + inode->i_op = &debugfs_symlink_inode_operations; inode->i_link = link; d_instantiate(dentry, inode); return end_creating(dentry);
Disallow opening of debugfs files that might be used to muck around when the kernel is locked down as various drivers give raw access to hardware through debugfs. Given the effort of auditing all 2000 or so files and manually fixing each one as necessary, I've chosen to apply a heuristic instead. The following changes are made: (1) chmod and chown are disallowed on debugfs objects (though the root dir can be modified by mount and remount, but I'm not worried about that). (2) When the kernel is locked down, only files with the following criteria are permitted to be opened: - The file must have mode 00444 - The file must not have ioctl methods - The file must not have mmap (3) When the kernel is locked down, files may only be opened for reading. Normal device interaction should be done through configfs, sysfs or a miscdev, not debugfs. Note that this makes it unnecessary to specifically lock down show_dsts(), show_devs() and show_call() in the asus-wmi driver. I would actually prefer to lock down all files by default and have the the files unlocked by the creator. This is tricky to manage correctly, though, as there are 19 creation functions and ~1600 call sites (some of them in loops scanning tables). Signed-off-by: David Howells <dhowells@redhat.com> cc: Andy Shevchenko <andy.shevchenko@gmail.com> cc: acpi4asus-user@lists.sourceforge.net cc: platform-driver-x86@vger.kernel.org cc: Matthew Garrett <mjg59@srcf.ucam.org> cc: Thomas Gleixner <tglx@linutronix.de> --- fs/debugfs/file.c | 28 ++++++++++++++++++++++++++++ fs/debugfs/inode.c | 30 ++++++++++++++++++++++++++++-- 2 files changed, 56 insertions(+), 2 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html