linux-kernel - Re: [PATCH V3] debugfs: allow access blktrace trace files in lockdown mode

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 19 Apr 2023 14:00:00 -0400
From:   Paul Moore <paul@...l-moore.com>
To:     Junxiao Bi <junxiao.bi@...cle.com>
Cc:     linux-kernel@...r.kernel.org,
        linux-security-module@...r.kernel.org, nathanl@...ux.ibm.com,
        axboe@...nel.dk, jmorris@...ei.org, serge@...lyn.com,
        konrad.wilk@...cle.com, joe.jin@...cle.com
Subject: Re: [PATCH V3] debugfs: allow access blktrace trace files in lockdown mode

On Tue, Apr 18, 2023 at 1:27 PM Junxiao Bi <junxiao.bi@...cle.com> wrote:
>
> blktrace trace files are per-cpu relay files that are used by kernel to
> export IO metadata(IO events, type, target disk, offset and len etc.) to
> userspace, no data from IO itself will be exported. These trace files have
> permission 0400, but mmap is supported, so they are blocked by lockdown.
> Skip lockdown for these files to allow blktrace work in lockdown mode.
>
> v3 <- v2:
> allow only blktrace trace file instead of relay files
> https://lore.kernel.org/lkml/b68c9e1d-71c8-adf9-f7da-1b56a3d4bfbc@oracle.com/T/
>
> v2 <- v1:
> Fix build error when CONFIG_RELAY is not defined.
> Reported-by: kernel test robot <lkp@...el.com>
> Link: https://lore.kernel.org/oe-kbuild-all/202304121714.6mahd9EW-lkp@intel.com/
> Signed-off-by: Junxiao Bi <junxiao.bi@...cle.com>
> ---
>  fs/debugfs/file.c            | 10 ++++++++++
>  include/linux/blktrace_api.h |  3 +++
>  include/linux/relay.h        |  3 +++
>  kernel/relay.c               | 16 ++++++++++++++++
>  kernel/trace/blktrace.c      |  7 +++++++
>  5 files changed, 39 insertions(+)
>
> diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
> index 1f971c880dde..973e38f3e8a1 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/blktrace_api.h>
>
>  #include "internal.h"
>
> @@ -142,6 +143,12 @@ 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:
> + * blktrace trace files are per-cpu relay files that are used by kernel to
> + * export IO metadata(IO events, type, target disk, offset and len etc.) to
> + * userspace, no data from IO itself will be exported. These trace files have
> + * permission 0400, but mmap is supported, so they are blocked by lockdown.
> + * Skip lockdown for these files to allow blktrace work in lockdown mode.
>   */
>  static int debugfs_locked_down(struct inode *inode,
>                                struct file *filp,
> @@ -154,6 +161,9 @@ static int debugfs_locked_down(struct inode *inode,
>             !real_fops->mmap)
>                 return 0;
>
> +       if (blk_trace_is_tracefile(inode, real_fops))
> +               return 0;

I think it would be a little more foolproof if we made the connection
to lockdown a bit more explicit in the relay/blktrace code.  How about
something like this here, borrowing your previously defined
'is_relay_file()' function:

  if (is_relay_file(real_fops) && relay_bypass_lockdown(inode, real_fops))
    return 0;

... and in the relay code we would have something like this, borrowing
from your logic in this patch, and using some shortcut-y pseudo-code:

  bool relay_bypass_lockdown(struct inode *inode,
                             const struct file_operations *fops)
  {
    struct rchan_buf *buf = inode->i_private;

    if (buf->chan->cb->bypass_lockdown)
      return buf->chan->cb->bypass_lockdown(inode);

    return false;
  }

... where in the case of blktrace rchan_callbacks::bypass_lockdown
would be a simple "true", assuming it is safe to do so (we need some
ACKs from the blktrace folks):

  bool blk_trace_bypass_lockdown(struct inode *inode)
  {
    return true;
  }

-- 
paul-moore.com