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

[Junxiao Bi]

> On Apr 19, 2023, at 11:00 AM, Paul Moore <paul@xxxxxxxxxxxxxx> wrote:
> 
> On Tue, Apr 18, 2023 at 1:27 PM Junxiao Bi <junxiao.bi@xxxxxxxxxx> 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@xxxxxxxxxx/T/
>> 
>> v2 <- v1:
>> Fix build error when CONFIG_RELAY is not defined.
>> Reported-by: kernel test robot <lkp@xxxxxxxxx>
>> Link: https://lore.kernel.org/oe-kbuild-all/202304121714.6mahd9EW-lkp@xxxxxxxxx/
>> Signed-off-by: Junxiao Bi <junxiao.bi@xxxxxxxxxx>
>> ---
>> 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;
>  }
Good idea. Will make a new version for it. Thanks.
> 
> -- 
> paul-moore.com