diff mbox series

[V2] debugfs: allow access relay files in lockdown mode

Message ID 20230412205316.7114-1-junxiao.bi@oracle.com
State New
Headers
Series [V2] debugfs: allow access relay files in lockdown mode |

Commit Message

Junxiao Bi April 12, 2023, 8:53 p.m. UTC 
  Relay files are used by kernel to transfer information to userspace, these
files have permission 0400, but mmap is supported, so they are blocked by
lockdown. But since kernel just generates the contents of those files while
not reading it, it is saft to access relay files in lockdown mode.

With this, blktrace can work well in lockdown mode.

v2 <- v1:
Fix build error when CONFIG_RELAY is not defined.
Reported-by: kernel test robot <lkp@intel.com>
Link: https://lore.kernel.org/oe-kbuild-all/202304121714.6mahd9EW-lkp@intel.com/

Signed-off-by: Junxiao Bi <junxiao.bi@oracle.com>
---
 fs/debugfs/file.c     | 9 +++++++++
 include/linux/relay.h | 8 ++++++++
 2 files changed, 17 insertions(+)

Comments

Nathan Lynch April 17, 2023, 8:34 p.m. UTC | #1 
Junxiao Bi <junxiao.bi@oracle.com> writes:
> Relay files are used by kernel to transfer information to userspace, these
> files have permission 0400, but mmap is supported, so they are blocked by
> lockdown. But since kernel just generates the contents of those files while
> not reading it, it is saft to access relay files in lockdown mode.
>
> With this, blktrace can work well in lockdown mode.

Assuming that all relay users do not expose the kinds of information
that confidentiality mode tries to restrict, this change seems OK to
me. I think that assumption applies to blktrace; apart from that, there
is a handful of drivers that use relay files (I searched for
relay_open() call sites, maybe there is a better way).


> v2 <- v1:
> Fix build error when CONFIG_RELAY is not defined.
> Reported-by: kernel test robot <lkp@intel.com>
> Link: https://lore.kernel.org/oe-kbuild-all/202304121714.6mahd9EW-lkp@intel.com/
>
> Signed-off-by: Junxiao Bi <junxiao.bi@oracle.com>
> ---
>  fs/debugfs/file.c     | 9 +++++++++
>  include/linux/relay.h | 8 ++++++++
>  2 files changed, 17 insertions(+)
>
> diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
> index 1f971c880dde..c52c94e20366 100644
> --- a/fs/debugfs/file.c
> +++ b/fs/debugfs/file.c
> @@ -21,6 +21,7 @@
>  #include <linux/pm_runtime.h>
>  #include <linux/poll.h>
>  #include <linux/security.h>
> +#include <linux/relay.h>
>  
>  #include "internal.h"
>  
> @@ -142,6 +143,11 @@ 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.
> + * Exception:
> + * Relay files are used by kernel to transfer information to userspace, these
> + * files have permission 0400, but mmap is supported, so they are blocked by
> + * lockdown. But since kernel just generates the contents of those files while
> + * not reading it, it is saft to access relay files in lockdown mode.
>   */
>  static int debugfs_locked_down(struct inode *inode,
>  			       struct file *filp,
> @@ -154,6 +160,9 @@ static int debugfs_locked_down(struct inode *inode,
>  	    !real_fops->mmap)
>  		return 0;
>  
> +	if (is_relay_file(real_fops))
> +		return 0;
> +
>  	if (security_locked_down(LOCKDOWN_DEBUGFS))
>  		return -EPERM;
>  
> diff --git a/include/linux/relay.h b/include/linux/relay.h
> index 72b876dd5cb8..914f116d826e 100644
> --- a/include/linux/relay.h
> +++ b/include/linux/relay.h
> @@ -279,8 +279,16 @@ extern const struct file_operations relay_file_operations;
>  
>  #ifdef CONFIG_RELAY
>  int relay_prepare_cpu(unsigned int cpu);
> +static inline int is_relay_file(const struct file_operations *fops)
> +{
> +	return fops == &relay_file_operations;
> +}
>  #else
>  #define relay_prepare_cpu     NULL
> +static inline int is_relay_file(const struct file_operations *fops)
> +{
> +	return 0;
> +}

The return type should be bool for predicate functions like this IMO.
Paul Moore April 17, 2023, 9:56 p.m. UTC | #2 
On Mon, Apr 17, 2023 at 4:39 PM Nathan Lynch <nathanl@linux.ibm.com> wrote:
> Junxiao Bi <junxiao.bi@oracle.com> writes:
> > Relay files are used by kernel to transfer information to userspace, these
> > files have permission 0400, but mmap is supported, so they are blocked by
> > lockdown. But since kernel just generates the contents of those files while
> > not reading it, it is saft to access relay files in lockdown mode.
> >
> > With this, blktrace can work well in lockdown mode.
>
> Assuming that all relay users do not expose the kinds of information
> that confidentiality mode tries to restrict, this change seems OK to
> me. I think that assumption applies to blktrace; apart from that, there
> is a handful of drivers that use relay files (I searched for
> relay_open() call sites, maybe there is a better way).

At the very least I see an Intel graphics driver and some network
drivers, but like you, that was a quick search and I'm probably
missing something.  At the very least someone needs to go audit those
users/drivers to ensure this is safe to merge.

However, regardless of what that code audit may turn up, I'm a little
concerned that it would be all too easy to add a new relay interface
user which isn't safe.  The check in debugfs_locked_down() is far too
removed from the code which is using the relay interface for it to be
likely noticed in a future case where an unsafe user is added.  This
looks like a vulnerability waiting to happen.
Junxiao Bi April 17, 2023, 11:48 p.m. UTC | #3 
On 4/17/23 2:56 PM, Paul Moore wrote:

> On Mon, Apr 17, 2023 at 4:39 PM Nathan Lynch<nathanl@linux.ibm.com>  wrote:
>> Junxiao Bi<junxiao.bi@oracle.com>  writes:
>>> Relay files are used by kernel to transfer information to userspace, these
>>> files have permission 0400, but mmap is supported, so they are blocked by
>>> lockdown. But since kernel just generates the contents of those files while
>>> not reading it, it is saft to access relay files in lockdown mode.
>>>
>>> With this, blktrace can work well in lockdown mode.
>> Assuming that all relay users do not expose the kinds of information
>> that confidentiality mode tries to restrict, this change seems OK to
>> me. I think that assumption applies to blktrace; apart from that, there
>> is a handful of drivers that use relay files (I searched for
>> relay_open() call sites, maybe there is a better way).
> At the very least I see an Intel graphics driver and some network
> drivers, but like you, that was a quick search and I'm probably
> missing something.  At the very least someone needs to go audit those
> users/drivers to ensure this is safe to merge.
>
> However, regardless of what that code audit may turn up, I'm a little
> concerned that it would be all too easy to add a new relay interface
> user which isn't safe.  The check in debugfs_locked_down() is far too
> removed from the code which is using the relay interface for it to be
> likely noticed in a future case where an unsafe user is added.  This
> looks like a vulnerability waiting to happen.

I got this concern. I will make a new version to limit it to only allow 
blktrace trace files.

Thanks,

Junxiao.
diff mbox series

Patch

diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
index 1f971c880dde..c52c94e20366 100644
--- a/fs/debugfs/file.c
+++ b/fs/debugfs/file.c
@@ -21,6 +21,7 @@ 
 #include <linux/pm_runtime.h>
 #include <linux/poll.h>
 #include <linux/security.h>
+#include <linux/relay.h>
 
 #include "internal.h"
 
@@ -142,6 +143,11 @@  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.
+ * Exception:
+ * Relay files are used by kernel to transfer information to userspace, these
+ * files have permission 0400, but mmap is supported, so they are blocked by
+ * lockdown. But since kernel just generates the contents of those files while
+ * not reading it, it is saft to access relay files in lockdown mode.
  */
 static int debugfs_locked_down(struct inode *inode,
 			       struct file *filp,
@@ -154,6 +160,9 @@  static int debugfs_locked_down(struct inode *inode,
 	    !real_fops->mmap)
 		return 0;
 
+	if (is_relay_file(real_fops))
+		return 0;
+
 	if (security_locked_down(LOCKDOWN_DEBUGFS))
 		return -EPERM;
 
diff --git a/include/linux/relay.h b/include/linux/relay.h
index 72b876dd5cb8..914f116d826e 100644
--- a/include/linux/relay.h
+++ b/include/linux/relay.h
@@ -279,8 +279,16 @@  extern const struct file_operations relay_file_operations;
 
 #ifdef CONFIG_RELAY
 int relay_prepare_cpu(unsigned int cpu);
+static inline int is_relay_file(const struct file_operations *fops)
+{
+	return fops == &relay_file_operations;
+}
 #else
 #define relay_prepare_cpu     NULL
+static inline int is_relay_file(const struct file_operations *fops)
+{
+	return 0;
+}
 #endif
 
 #endif /* _LINUX_RELAY_H */