Relay files in debugfs is used to export information from kernel to
userspace, relay user should tell whether the exported information
is safe to access in lockdown mode or not. The access will be denied
by default.
v4 <- v3:
refactor the code to make code more foolproof.
v3 <- v2:
allow only blktrace trace file instead of relay files
https://lore.kernel.org/all/[email protected]/
v2 <- v1:
Fix build error when CONFIG_RELAY is not defined.
Reported-by: kernel test robot <[email protected]>
Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/
Signed-off-by: Junxiao Bi <[email protected]>
---
fs/debugfs/file.c | 11 +++++++++++
include/linux/relay.h | 16 ++++++++++++++++
kernel/relay.c | 17 +++++++++++++++++
3 files changed, 44 insertions(+)
diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
index 1f971c880dde..e12dd8634dc6 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,13 @@ 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 in debugfs are used by kernel to transfer data from kernel to
+ * userspace, these files are with permission 0400, but mmap is supported, so
+ * the access is blocked by default. But there are relay users like blktrace
+ * which will only export none security sensitive info to userspace, so provide
+ * a way for relay user to hook in to tell whether accessing the file in lockdown
+ * mode is safe or not.
*/
static int debugfs_locked_down(struct inode *inode,
struct file *filp,
@@ -154,6 +162,9 @@ static int debugfs_locked_down(struct inode *inode,
!real_fops->mmap)
return 0;
+ if (relay_bypass_lockdown(inode, 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..0fd1fb638434 100644
--- a/include/linux/relay.h
+++ b/include/linux/relay.h
@@ -147,6 +147,19 @@ struct rchan_callbacks
* This callback is mandatory.
*/
int (*remove_buf_file)(struct dentry *dentry);
+
+ /*
+ * bypass_lockdown - check whether file can be accessed in lockdown mode.
+ *
+ * Called during file open. It depends on each relay user to tell whether
+ * accessing the file is safe or not in lockdown mode.
+ *
+ * The callback should return "true" if allowing access, "false" if not.
+ *
+ * This callback is optional. If not set, accessing the file will be
+ * denied in lockdown mode.
+ */
+ bool (*bypass_lockdown)(struct inode *inode);
};
/*
@@ -279,8 +292,11 @@ extern const struct file_operations relay_file_operations;
#ifdef CONFIG_RELAY
int relay_prepare_cpu(unsigned int cpu);
+extern bool relay_bypass_lockdown(struct inode *inode,
+ const struct file_operations *fops);
#else
#define relay_prepare_cpu NULL
+#define relay_bypass_lockdown(inode, fops) (false)
#endif
#endif /* _LINUX_RELAY_H */
diff --git a/kernel/relay.c b/kernel/relay.c
index 9aa70ae53d24..d5a76566ef62 100644
--- a/kernel/relay.c
+++ b/kernel/relay.c
@@ -1234,6 +1234,23 @@ static ssize_t relay_file_splice_read(struct file *in,
return ret;
}
+bool relay_bypass_lockdown(struct inode *inode,
+ const struct file_operations *fops)
+{
+ struct rchan_buf *buf;
+
+ /* Not a relay file */
+ if (fops != &relay_file_operations)
+ return false;
+
+ buf = (struct rchan_buf *)inode->i_private;
+ if (!buf->chan->cb->bypass_lockdown)
+ return false;
+
+ return buf->chan->cb->bypass_lockdown(inode);
+}
+EXPORT_SYMBOL_GPL(relay_bypass_lockdown);
+
const struct file_operations relay_file_operations = {
.open = relay_file_open,
.poll = relay_file_poll,
--
2.24.3 (Apple Git-128)
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. Bypass lockdown for
these files will make blktrace work in lockdown mode.
Signed-off-by: Junxiao Bi <[email protected]>
---
kernel/trace/blktrace.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c
index d5d94510afd3..e1a9f8b7d710 100644
--- a/kernel/trace/blktrace.c
+++ b/kernel/trace/blktrace.c
@@ -490,10 +490,16 @@ static struct dentry *blk_create_buf_file_callback(const char *filename,
&relay_file_operations);
}
+static bool blk_bypass_lockdown(struct inode *inode)
+{
+ return true;
+}
+
static const struct rchan_callbacks blk_relay_callbacks = {
.subbuf_start = blk_subbuf_start_callback,
.create_buf_file = blk_create_buf_file_callback,
.remove_buf_file = blk_remove_buf_file_callback,
+ .bypass_lockdown = blk_bypass_lockdown,
};
static void blk_trace_setup_lba(struct blk_trace *bt,
--
2.24.3 (Apple Git-128)
Any IO folks can help review this patch?
Paul needs a confirm from you that the information blktrace exporting to
userspace through the relay files are safe, not leaking information that
userspace shouldn't know in lockdown mode.
Thanks,
Junxiao.
On 4/20/23 2:53 PM, Junxiao Bi 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. Bypass lockdown for
> these files will make blktrace work in lockdown mode.
>
> Signed-off-by: Junxiao Bi <[email protected]>
> ---
> kernel/trace/blktrace.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c
> index d5d94510afd3..e1a9f8b7d710 100644
> --- a/kernel/trace/blktrace.c
> +++ b/kernel/trace/blktrace.c
> @@ -490,10 +490,16 @@ static struct dentry *blk_create_buf_file_callback(const char *filename,
> &relay_file_operations);
> }
>
> +static bool blk_bypass_lockdown(struct inode *inode)
> +{
> + return true;
> +}
> +
> static const struct rchan_callbacks blk_relay_callbacks = {
> .subbuf_start = blk_subbuf_start_callback,
> .create_buf_file = blk_create_buf_file_callback,
> .remove_buf_file = blk_remove_buf_file_callback,
> + .bypass_lockdown = blk_bypass_lockdown,
> };
>
> static void blk_trace_setup_lba(struct blk_trace *bt,
On 4/25/23 11:55?AM, Junxiao Bi wrote:
> Any IO folks can help review this patch?
>
> Paul needs a confirm from you that the information blktrace exporting
> to userspace through the relay files are safe, not leaking information
> that userspace shouldn't know in lockdown mode.
I don't know anything about what lockdown is, but in terms of blktrace,
it is a way to trace meta data associated with IO. It'll tell you things
like "task T wants to {read,write} on device D, at offset X, and of size
Y". For passthrough IO, it'll also dump the CDB. There's never any
actual data traced.
--
Jens Axboe
On 4/25/23 12:12 PM, Jens Axboe wrote:
> On 4/25/23 11:55?AM, Junxiao Bi wrote:
>> Any IO folks can help review this patch?
>>
>> Paul needs a confirm from you that the information blktrace exporting
>> to userspace through the relay files are safe, not leaking information
>> that userspace shouldn't know in lockdown mode.
> I don't know anything about what lockdown is, but in terms of blktrace,
> it is a way to trace meta data associated with IO. It'll tell you things
> like "task T wants to {read,write} on device D, at offset X, and of size
> Y". For passthrough IO, it'll also dump the CDB. There's never any
> actual data traced.
Thanks Jens. Lockdown is a security feature which is trying to limit
user's(including root) capability to access security sensitive
information inside kernel, there are two modes, read-only access is
allowed in "integrity" mode while both read and write access are
disabled in "confidentiality" mode.
Paul, do you have any other concerns regarding blktrace? As Jens
mentioned, blktrace just exported IO metadata to Userspace, those were
not security sensitive information.
Thanks,
Junxiao.
On Wed, Apr 26, 2023 at 12:33 PM Junxiao Bi <[email protected]> wrote:
> Paul, do you have any other concerns regarding blktrace? As Jens
> mentioned, blktrace just exported IO metadata to Userspace, those were
> not security sensitive information.
I think this version of the patchset is much better, thanks for your
patience. I don't have any further concerns, and since the lockdown
LSM doesn't have a dedicated maintainer I think you should be all set
from my perspective.
Since there are no changes under security/, I'm assuming this will go
in via the tracing tree?
--
paul-moore.com
On 4/28/23 2:26 PM, Paul Moore wrote:
> On Wed, Apr 26, 2023 at 12:33 PM Junxiao Bi <[email protected]> wrote:
>> Paul, do you have any other concerns regarding blktrace? As Jens
>> mentioned, blktrace just exported IO metadata to Userspace, those were
>> not security sensitive information.
> I think this version of the patchset is much better, thanks for your
> patience. I don't have any further concerns, and since the lockdown
> LSM doesn't have a dedicated maintainer I think you should be all set
> from my perspective.
Thanks a lot for the review.
>
> Since there are no changes under security/, I'm assuming this will go
> in via the tracing tree?
Should I notify some specific maintainer or mail list for merging?
Thanks,
Junxiao.
>
On Fri, Apr 28, 2023 at 6:41 PM Junxiao Bi <[email protected]> wrote:
> On 4/28/23 2:26 PM, Paul Moore wrote:
> > On Wed, Apr 26, 2023 at 12:33 PM Junxiao Bi <[email protected]> wrote:
> >> Paul, do you have any other concerns regarding blktrace? As Jens
> >> mentioned, blktrace just exported IO metadata to Userspace, those were
> >> not security sensitive information.
> > I think this version of the patchset is much better, thanks for your
> > patience. I don't have any further concerns, and since the lockdown
> > LSM doesn't have a dedicated maintainer I think you should be all set
> > from my perspective.
>
> Thanks a lot for the review.
>
> > Since there are no changes under security/, I'm assuming this will go
> > in via the tracing tree?
>
> Should I notify some specific maintainer or mail list for merging?
When in doubt, the scripts/get_maintainer.pl tool in the kernel
sources can be helpful.
The results for the debugfs and relay files are fairly generic, but if
you look at the results for the blktrace.c file you get a more
reasonable list of maintainers and mailing lists. I believe Jens has
already commented on your patches at least once, I don't recall if the
others have or not. I see you've already CC'd the block mailing list,
so that might be enough.
Keep in mind that we are in the middle of a merge window so it is very
possible this patch might not be merged in a working/next/etc. branch
until after the merge window closes (every maintainer is a little bit
different in this regard).
--
paul-moore.com
On 4/30/23 2:46 PM, Paul Moore wrote:
> On Fri, Apr 28, 2023 at 6:41 PM Junxiao Bi <[email protected]> wrote:
>> On 4/28/23 2:26 PM, Paul Moore wrote:
>>> On Wed, Apr 26, 2023 at 12:33 PM Junxiao Bi <[email protected]> wrote:
>>>> Paul, do you have any other concerns regarding blktrace? As Jens
>>>> mentioned, blktrace just exported IO metadata to Userspace, those were
>>>> not security sensitive information.
>>> I think this version of the patchset is much better, thanks for your
>>> patience. I don't have any further concerns, and since the lockdown
>>> LSM doesn't have a dedicated maintainer I think you should be all set
>>> from my perspective.
>> Thanks a lot for the review.
>>
>>> Since there are no changes under security/, I'm assuming this will go
>>> in via the tracing tree?
>> Should I notify some specific maintainer or mail list for merging?
> When in doubt, the scripts/get_maintainer.pl tool in the kernel
> sources can be helpful.
>
> The results for the debugfs and relay files are fairly generic, but if
> you look at the results for the blktrace.c file you get a more
> reasonable list of maintainers and mailing lists. I believe Jens has
> already commented on your patches at least once, I don't recall if the
> others have or not. I see you've already CC'd the block mailing list,
> so that might be enough.
>
> Keep in mind that we are in the middle of a merge window so it is very
> possible this patch might not be merged in a working/next/etc. branch
> until after the merge window closes (every maintainer is a little bit
> different in this regard).
I didn't see the patches in the trace tree yet, maybe better to merge it
through block tree since it's a blktrace fix.
Jens, can you help merge these two patches to your tree?
Thanks,
Junxiao.
>
Hi Paul,
The patches have not been merged yet, i would like to resend them, just
want to confirm i can add your Reviewed-by in the patches, right?
Thanks,
Junxiao.
On 5/9/23 9:13 AM, Junxiao Bi wrote:
> On 4/30/23 2:46 PM, Paul Moore wrote:
>
>> On Fri, Apr 28, 2023 at 6:41 PM Junxiao Bi <[email protected]>
>> wrote:
>>> On 4/28/23 2:26 PM, Paul Moore wrote:
>>>> On Wed, Apr 26, 2023 at 12:33 PM Junxiao Bi <[email protected]>
>>>> wrote:
>>>>> Paul, do you have any other concerns regarding blktrace? As Jens
>>>>> mentioned, blktrace just exported IO metadata to Userspace, those
>>>>> were
>>>>> not security sensitive information.
>>>> I think this version of the patchset is much better, thanks for your
>>>> patience. I don't have any further concerns, and since the lockdown
>>>> LSM doesn't have a dedicated maintainer I think you should be all set
>>>> from my perspective.
>>> Thanks a lot for the review.
>>>
>>>> Since there are no changes under security/, I'm assuming this will go
>>>> in via the tracing tree?
>>> Should I notify some specific maintainer or mail list for merging?
>> When in doubt, the scripts/get_maintainer.pl tool in the kernel
>> sources can be helpful.
>>
>> The results for the debugfs and relay files are fairly generic, but if
>> you look at the results for the blktrace.c file you get a more
>> reasonable list of maintainers and mailing lists. I believe Jens has
>> already commented on your patches at least once, I don't recall if the
>> others have or not. I see you've already CC'd the block mailing list,
>> so that might be enough.
>>
>> Keep in mind that we are in the middle of a merge window so it is very
>> possible this patch might not be merged in a working/next/etc. branch
>> until after the merge window closes (every maintainer is a little bit
>> different in this regard).
>
> I didn't see the patches in the trace tree yet, maybe better to merge
> it through block tree since it's a blktrace fix.
>
> Jens, can you help merge these two patches to your tree?
>
> Thanks,
>
> Junxiao.
>
>>
On Fri, May 26, 2023 at 12:56 PM Junxiao Bi <[email protected]> wrote:
>
> Hi Paul,
>
> The patches have not been merged yet, i would like to resend them, just
> want to confirm i can add your Reviewed-by in the patches, right?
Hi Junxiao,
I haven't personally had the time to verify the blktrace claims that
it doesn't violate the Lockdown principles so I'm not comfortable
adding my reviewed-by tag at this point in time, I'm sorry.
> On 5/9/23 9:13 AM, Junxiao Bi wrote:
> > On 4/30/23 2:46 PM, Paul Moore wrote:
> >
> >> On Fri, Apr 28, 2023 at 6:41 PM Junxiao Bi <[email protected]>
> >> wrote:
> >>> On 4/28/23 2:26 PM, Paul Moore wrote:
> >>>> On Wed, Apr 26, 2023 at 12:33 PM Junxiao Bi <[email protected]>
> >>>> wrote:
> >>>>> Paul, do you have any other concerns regarding blktrace? As Jens
> >>>>> mentioned, blktrace just exported IO metadata to Userspace, those
> >>>>> were
> >>>>> not security sensitive information.
> >>>> I think this version of the patchset is much better, thanks for your
> >>>> patience. I don't have any further concerns, and since the lockdown
> >>>> LSM doesn't have a dedicated maintainer I think you should be all set
> >>>> from my perspective.
> >>> Thanks a lot for the review.
> >>>
> >>>> Since there are no changes under security/, I'm assuming this will go
> >>>> in via the tracing tree?
> >>> Should I notify some specific maintainer or mail list for merging?
> >> When in doubt, the scripts/get_maintainer.pl tool in the kernel
> >> sources can be helpful.
> >>
> >> The results for the debugfs and relay files are fairly generic, but if
> >> you look at the results for the blktrace.c file you get a more
> >> reasonable list of maintainers and mailing lists. I believe Jens has
> >> already commented on your patches at least once, I don't recall if the
> >> others have or not. I see you've already CC'd the block mailing list,
> >> so that might be enough.
> >>
> >> Keep in mind that we are in the middle of a merge window so it is very
> >> possible this patch might not be merged in a working/next/etc. branch
> >> until after the merge window closes (every maintainer is a little bit
> >> different in this regard).
> >
> > I didn't see the patches in the trace tree yet, maybe better to merge
> > it through block tree since it's a blktrace fix.
> >
> > Jens, can you help merge these two patches to your tree?
> >
> > Thanks,
> >
> > Junxiao.
--
paul-moore.com
On 5/26/23 2:37 PM, Paul Moore wrote:
> On Fri, May 26, 2023 at 12:56 PM Junxiao Bi <[email protected]> wrote:
>> Hi Paul,
>>
>> The patches have not been merged yet, i would like to resend them, just
>> want to confirm i can add your Reviewed-by in the patches, right?
> Hi Junxiao,
>
> I haven't personally had the time to verify the blktrace claims that
> it doesn't violate the Lockdown principles so I'm not comfortable
> adding my reviewed-by tag at this point in time, I'm sorry.
No problem. With Jens confirmed blktrace only exposed IO metadata to
userspace, if any more query regarding blktrace, please let me know.
Thank you.
Thanks,
Junxiao.
>
>> On 5/9/23 9:13 AM, Junxiao Bi wrote:
>>> On 4/30/23 2:46 PM, Paul Moore wrote:
>>>
>>>> On Fri, Apr 28, 2023 at 6:41 PM Junxiao Bi <[email protected]>
>>>> wrote:
>>>>> On 4/28/23 2:26 PM, Paul Moore wrote:
>>>>>> On Wed, Apr 26, 2023 at 12:33 PM Junxiao Bi <[email protected]>
>>>>>> wrote:
>>>>>>> Paul, do you have any other concerns regarding blktrace? As Jens
>>>>>>> mentioned, blktrace just exported IO metadata to Userspace, those
>>>>>>> were
>>>>>>> not security sensitive information.
>>>>>> I think this version of the patchset is much better, thanks for your
>>>>>> patience. I don't have any further concerns, and since the lockdown
>>>>>> LSM doesn't have a dedicated maintainer I think you should be all set
>>>>>> from my perspective.
>>>>> Thanks a lot for the review.
>>>>>
>>>>>> Since there are no changes under security/, I'm assuming this will go
>>>>>> in via the tracing tree?
>>>>> Should I notify some specific maintainer or mail list for merging?
>>>> When in doubt, the scripts/get_maintainer.pl tool in the kernel
>>>> sources can be helpful.
>>>>
>>>> The results for the debugfs and relay files are fairly generic, but if
>>>> you look at the results for the blktrace.c file you get a more
>>>> reasonable list of maintainers and mailing lists. I believe Jens has
>>>> already commented on your patches at least once, I don't recall if the
>>>> others have or not. I see you've already CC'd the block mailing list,
>>>> so that might be enough.
>>>>
>>>> Keep in mind that we are in the middle of a merge window so it is very
>>>> possible this patch might not be merged in a working/next/etc. branch
>>>> until after the merge window closes (every maintainer is a little bit
>>>> different in this regard).
>>> I didn't see the patches in the trace tree yet, maybe better to merge
>>> it through block tree since it's a blktrace fix.
>>>
>>> Jens, can you help merge these two patches to your tree?
>>>
>>> Thanks,
>>>
>>> Junxiao.