On Mon, May 26, 2025 at 07:37:52PM +0100, Tingmao Wang wrote:

On 5/23/25 17:57, Mickaël Salaün wrote:

quoted

Add a tracepoint for Landlock path_beneath rule addition.  This is
useful to tie a Landlock object with a file for debug purpose.

Allocate the absolute path names when adding new rules.  This is OK
because landlock_add_rule(2) is not a performance critical code.

Here is an example of landlock_add_rule_fs traces:
   ruleset=0x000000007e3b1c4a key=inode:0xffff888004f59260 allowed=0xd dev=0:16 ino=306 path=/usr
   ruleset=0x000000007e3b1c4a key=inode:0xffff888004f59240 allowed=0xffff dev=0:16 ino=346 path=/root

TODO: Use Landlock IDs instead of kernel addresses to identify Landlock
objects (e.g. inode).

Do you mean like the u64 domain ID for audit, but for objects?  Since there
currently isn't a u64, non-pointer ID, I guess we would have to create one
for the object just for tracing?

Yes, this is the idea.  Landlock objects are scarce so it should not
change much.  Another advantage of using a Landlock ID is that there is
no risk for confusion (i.e. they are not recycled).  I'm not sure if
it's worth it though.

My understanding is that kernel pointers are not hidden from the root user /
someone who can read traces, so maybe just printing the pointer is good
enough?

In theory printed kernel pointers should be hashed, but I guess in
practice and especially with eBPF, kernel addresses may not be really
hidden.

On the other hand, providing a pointer to an eBPF program enables us to
inspect the related data structure.  However, such pointer cannot be
dereferenced in TP_printk() because it is called after the tracepoint.

In fact, I guess we should probably provide kernel addresses (to the
trace context), but I'm not sure if we should print IDs or just kernel
addresses.  It might be handy to easily map a Landlock domain pointer to
its ID in the audit log, but that would require to also copy IDs in the
trace context...  It looks like this is how works sock tracepoints (e.g.
skaddr, but I'm not sure if void * is only there to avoid dereferencing
this pointer in TP_printk).

My understanding is that with eBPF we can read a kernel address from the
trace context without race condition wrt TP_print() which may be called
when the address was already recycled (which could lead to misleading
concurrent traces).

Steven, is it correct? Any advice?

quoted

Cc: Günther Noack <gnoack@google.com>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Tingmao Wang <redacted>
Signed-off-by: Mickaël Salaün <mic@digikod.net>
---
  MAINTAINERS                     |  1 +
  include/trace/events/landlock.h | 68 +++++++++++++++++++++++++++++++++
  security/landlock/Makefile      | 11 +++++-
  security/landlock/fs.c          | 22 +++++++++++
  security/landlock/fs.h          |  3 ++
  security/landlock/trace.c       | 14 +++++++
  6 files changed, 117 insertions(+), 2 deletions(-)
  create mode 100644 include/trace/events/landlock.h
  create mode 100644 security/landlock/trace.c

quoted

[...]

diff --git a/security/landlock/fs.c b/security/landlock/fs.c

index 73a20a501c3c..e5d673240882 100644

--- a/security/landlock/fs.c
+++ b/security/landlock/fs.c

@@ -36,6 +36,7 @@
  #include <linux/types.h>
  #include <linux/wait_bit.h>
  #include <linux/workqueue.h>
+#include <trace/events/landlock.h>
  #include <uapi/linux/fiemap.h>
  #include <uapi/linux/landlock.h>

@@ -345,6 +346,27 @@ int landlock_append_fs_rule(struct landlock_ruleset *const ruleset,
  	mutex_lock(&ruleset->lock);
  	err = landlock_insert_rule(ruleset, ref, access_rights);
  	mutex_unlock(&ruleset->lock);
+
+	if (!err && trace_landlock_add_rule_fs_enabled()) {
+		const char *pathname;
+		/* Does not handle deleted files. */
+		char *buffer __free(__putname) = __getname();
+
+		if (buffer) {
+			const char *absolute_path =
+				d_absolute_path(path, buffer, PATH_MAX);
+			if (!IS_ERR_OR_NULL(absolute_path))
+				pathname = absolute_path;
+			else
+				pathname = "<too_long>";

Not sure if it's necessary to go that far, but I think d_absolute_path
returns -ENAMETOOLONG in the too long case, and -EINVAL in the "not possible
to construct a path" case (I guess e.g. if it's an anonymous file or
detached mount).  We could add an else if branch to check which case it is
and use different strings.

I mimicked the audit behavior but we can indeed add another case.

quoted

+		} else {
+			/* Same format as audit_log_d_path(). */
+			pathname = "<no_memory>";
+		}
+		trace_landlock_add_rule_fs(ruleset, &ref, access_rights, path,
+					   pathname);
+	}
+
  	/*
  	 * No need to check for an error because landlock_insert_rule()
  	 * increments the refcount for the new object if needed.

diff --git a/security/landlock/fs.h b/security/landlock/fs.h
index bf9948941f2f..60be95ebfb0b 100644
--- a/security/landlock/fs.h
+++ b/security/landlock/fs.h

@@ -11,6 +11,7 @@
  #define _SECURITY_LANDLOCK_FS_H
  #include <linux/build_bug.h>
+#include <linux/cleanup.h>
  #include <linux/fs.h>
  #include <linux/init.h>
  #include <linux/rcupdate.h>

@@ -128,4 +129,6 @@ int landlock_append_fs_rule(struct landlock_ruleset *const ruleset,
  			    const struct path *const path,
  			    access_mask_t access_hierarchy);
+DEFINE_FREE(__putname, char *, if (_T) __putname(_T))

Out of curiosity why not put this in include/linux/fs.h (seems to compile
for me when added there)?

I moved it here for this RFC but the next patch series will put it in
linux/fs.h

quoted

+
  #endif /* _SECURITY_LANDLOCK_FS_H */

diff --git a/security/landlock/trace.c b/security/landlock/trace.c
new file mode 100644
index 000000000000..98874cda473b
--- /dev/null
+++ b/security/landlock/trace.c

@@ -0,0 +1,14 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Landlock - Tracepoints
+ *
+ * Copyright © 2025 Microsoft Corporation
+ */
+
+#include <linux/path.h>
+
+#include "access.h"
+#include "ruleset.h"
+
+#define CREATE_TRACE_POINTS
+#include <trace/events/landlock.h>