[PATCH 0/1] security: Propagate universal pointer data in bpf hooks

[PATCH 0/1] security: Propagate universal pointer data in bpf hooks

[Blaise Boscaccy]

Hello,

While trying to implement an eBPF gatekeeper program, we ran into an
issue whereas the LSM hooks are missing some relevant data.

Certain subcommands passed to the bpf() syscall can be invoked from
either the kernel or userspace. Additionally, some fields in the
bpf_attr sruct contain pointers, and depending on where the subcommand
was invoked, could point to either user or kernel memory. One example
of this, is the bpf_prog_load subcommand and it's fd_array.  This data
is made available and used by the verifier, but not made available to
the LSM subsystem. This patchset simply exposes that information to
applicable LSM hooks.


Blaise Boscaccy (1):
  security: Propagate universal pointer data in bpf hooks

 include/linux/lsm_hook_defs.h |  6 +++---
 include/linux/security.h      | 13 +++++++------
 kernel/bpf/syscall.c          | 10 +++++-----
 security/security.c           | 17 ++++++++++-------
 security/selinux/hooks.c      |  6 +++---
 5 files changed, 28 insertions(+), 24 deletions(-)

-- 
2.48.1

[PATCH 1/1] security: Propagate universal pointer data in bpf hooks

[Blaise Boscaccy]

Certain bpf syscall subcommands are available for usage from both
userspace and the kernel. LSM modules or eBPF gatekeeper programs may
need to take a different course of action depending on whether or not
a BPF syscall originated from the kernel or userspace.

Additionally, some of the bpf_attr struct fields contain pointers to
arbitrary memory. Currently the functionality to determine whether or
not a pointer refers to kernel memory or userspace memory is exposed
to the bpf verifier, but that information is missing from various LSM
hooks.

Here we augment the LSM hooks to provide this data, by simply passing
the corresponding universal pointer in any hook that contains already
contains a bpf_attr struct that corresponds to a subcommand that may
be called from the kernel.

Signed-off-by: Blaise Boscaccy <bboscaccy@linux.microsoft.com>
---
 include/linux/lsm_hook_defs.h |  6 +++---
 include/linux/security.h      | 13 +++++++------
 kernel/bpf/syscall.c          | 10 +++++-----
 security/security.c           | 17 ++++++++++-------
 security/selinux/hooks.c      |  6 +++---
 5 files changed, 28 insertions(+), 24 deletions(-)

diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index e2f1ce37c41ef..93e25d526d68d 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -426,14 +426,14 @@ LSM_HOOK(void, LSM_RET_VOID, audit_rule_free, void *lsmrule)
 #endif /* CONFIG_AUDIT */
 
 #ifdef CONFIG_BPF_SYSCALL
-LSM_HOOK(int, 0, bpf, int cmd, union bpf_attr *attr, unsigned int size)
+LSM_HOOK(int, 0, bpf, int cmd, union bpf_attr *attr, bpfptr_t uattr, unsigned int size)
 LSM_HOOK(int, 0, bpf_map, struct bpf_map *map, fmode_t fmode)
 LSM_HOOK(int, 0, bpf_prog, struct bpf_prog *prog)
 LSM_HOOK(int, 0, bpf_map_create, struct bpf_map *map, union bpf_attr *attr,
-	 struct bpf_token *token)
+	 bpfptr_t uattr, struct bpf_token *token)
 LSM_HOOK(void, LSM_RET_VOID, bpf_map_free, struct bpf_map *map)
 LSM_HOOK(int, 0, bpf_prog_load, struct bpf_prog *prog, union bpf_attr *attr,
-	 struct bpf_token *token)
+	 bpfptr_t uattr, struct bpf_token *token)
 LSM_HOOK(void, LSM_RET_VOID, bpf_prog_free, struct bpf_prog *prog)
 LSM_HOOK(int, 0, bpf_token_create, struct bpf_token *token, union bpf_attr *attr,
 	 const struct path *path)
diff --git a/include/linux/security.h b/include/linux/security.h
index 980b6c207cade..b6d82500b0d31 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -33,6 +33,7 @@
 #include <linux/mm.h>
 #include <linux/sockptr.h>
 #include <linux/bpf.h>
+#include <linux/bpfptr.h>
 #include <uapi/linux/lsm.h>
 #include <linux/lsm/selinux.h>
 #include <linux/lsm/smack.h>
@@ -2249,14 +2250,14 @@ struct bpf_map;
 struct bpf_prog;
 struct bpf_token;
 #ifdef CONFIG_SECURITY
-extern int security_bpf(int cmd, union bpf_attr *attr, unsigned int size);
+extern int security_bpf(int cmd, union bpf_attr *attr, bpfptr_t uattr, unsigned int size);
 extern int security_bpf_map(struct bpf_map *map, fmode_t fmode);
 extern int security_bpf_prog(struct bpf_prog *prog);
 extern int security_bpf_map_create(struct bpf_map *map, union bpf_attr *attr,
-				   struct bpf_token *token);
+				   bpfptr_t uattr, struct bpf_token *token);
 extern void security_bpf_map_free(struct bpf_map *map);
 extern int security_bpf_prog_load(struct bpf_prog *prog, union bpf_attr *attr,
-				  struct bpf_token *token);
+				  bpfptr_t uattr, struct bpf_token *token);
 extern void security_bpf_prog_free(struct bpf_prog *prog);
 extern int security_bpf_token_create(struct bpf_token *token, union bpf_attr *attr,
 				     const struct path *path);
@@ -2265,7 +2266,7 @@ extern int security_bpf_token_cmd(const struct bpf_token *token, enum bpf_cmd cm
 extern int security_bpf_token_capable(const struct bpf_token *token, int cap);
 #else
 static inline int security_bpf(int cmd, union bpf_attr *attr,
-					     unsigned int size)
+			       bpfptr_t uattr, unsigned int size)
 {
 	return 0;
 }
@@ -2281,7 +2282,7 @@ static inline int security_bpf_prog(struct bpf_prog *prog)
 }
 
 static inline int security_bpf_map_create(struct bpf_map *map, union bpf_attr *attr,
-					  struct bpf_token *token)
+					  bpfptr_t uattr, struct bpf_token *token)
 {
 	return 0;
 }
@@ -2290,7 +2291,7 @@ static inline void security_bpf_map_free(struct bpf_map *map)
 { }
 
 static inline int security_bpf_prog_load(struct bpf_prog *prog, union bpf_attr *attr,
-					 struct bpf_token *token)
+					 bpfptr_t uattr, struct bpf_token *token)
 {
 	return 0;
 }
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 2645540ae26a8..255b0dc83b49b 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -1306,7 +1306,7 @@ static bool bpf_net_capable(void)
 
 #define BPF_MAP_CREATE_LAST_FIELD map_token_fd
 /* called via syscall */
-static int map_create(union bpf_attr *attr)
+static int map_create(union bpf_attr *attr, bpfptr_t uattr)
 {
 	const struct bpf_map_ops *ops;
 	struct bpf_token *token = NULL;
@@ -1498,7 +1498,7 @@ static int map_create(union bpf_attr *attr)
 			attr->btf_vmlinux_value_type_id;
 	}
 
-	err = security_bpf_map_create(map, attr, token);
+	err = security_bpf_map_create(map, attr, uattr, token);
 	if (err)
 		goto free_map_sec;
 
@@ -2947,7 +2947,7 @@ static int bpf_prog_load(union bpf_attr *attr, bpfptr_t uattr, u32 uattr_size)
 	if (err < 0)
 		goto free_prog;
 
-	err = security_bpf_prog_load(prog, attr, token);
+	err = security_bpf_prog_load(prog, attr, uattr, token);
 	if (err)
 		goto free_prog_sec;
 
@@ -5773,13 +5773,13 @@ static int __sys_bpf(enum bpf_cmd cmd, bpfptr_t uattr, unsigned int size)
 	if (copy_from_bpfptr(&attr, uattr, size) != 0)
 		return -EFAULT;
 
-	err = security_bpf(cmd, &attr, size);
+	err = security_bpf(cmd, &attr, uattr, size);
 	if (err < 0)
 		return err;
 
 	switch (cmd) {
 	case BPF_MAP_CREATE:
-		err = map_create(&attr);
+		err = map_create(&attr, uattr);
 		break;
 	case BPF_MAP_LOOKUP_ELEM:
 		err = map_lookup_elem(&attr);
diff --git a/security/security.c b/security/security.c
index 143561ebc3e89..350e37c015820 100644
--- a/security/security.c
+++ b/security/security.c
@@ -5626,7 +5626,8 @@ int security_audit_rule_match(struct lsm_prop *prop, u32 field, u32 op,
  * security_bpf() - Check if the bpf syscall operation is allowed
  * @cmd: command
  * @attr: bpf attribute
- * @size: size
+ * @uattr: universal pointer for attr
+ * @size: size of bpf attribute
  *
  * Do a initial check for all bpf syscalls after the attribute is copied into
  * the kernel. The actual security module can implement their own rules to
@@ -5634,9 +5635,9 @@ int security_audit_rule_match(struct lsm_prop *prop, u32 field, u32 op,
  *
  * Return: Returns 0 if permission is granted.
  */
-int security_bpf(int cmd, union bpf_attr *attr, unsigned int size)
+int security_bpf(int cmd, union bpf_attr *attr, bpfptr_t uattr, unsigned int size)
 {
-	return call_int_hook(bpf, cmd, attr, size);
+	return call_int_hook(bpf, cmd, attr, uattr, size);
 }
 
 /**
@@ -5672,6 +5673,7 @@ int security_bpf_prog(struct bpf_prog *prog)
  * security_bpf_map_create() - Check if BPF map creation is allowed
  * @map: BPF map object
  * @attr: BPF syscall attributes used to create BPF map
+ * @uattr: universal pointer for attr
  * @token: BPF token used to grant user access
  *
  * Do a check when the kernel creates a new BPF map. This is also the
@@ -5680,15 +5682,16 @@ int security_bpf_prog(struct bpf_prog *prog)
  * Return: Returns 0 on success, error on failure.
  */
 int security_bpf_map_create(struct bpf_map *map, union bpf_attr *attr,
-			    struct bpf_token *token)
+			    bpfptr_t uattr, struct bpf_token *token)
 {
-	return call_int_hook(bpf_map_create, map, attr, token);
+	return call_int_hook(bpf_map_create, map, attr, uattr, token);
 }
 
 /**
  * security_bpf_prog_load() - Check if loading of BPF program is allowed
  * @prog: BPF program object
  * @attr: BPF syscall attributes used to create BPF program
+ * @uattr: universal pointer attribute
  * @token: BPF token used to grant user access to BPF subsystem
  *
  * Perform an access control check when the kernel loads a BPF program and
@@ -5698,9 +5701,9 @@ int security_bpf_map_create(struct bpf_map *map, union bpf_attr *attr,
  * Return: Returns 0 on success, error on failure.
  */
 int security_bpf_prog_load(struct bpf_prog *prog, union bpf_attr *attr,
-			   struct bpf_token *token)
+			   bpfptr_t uattr, struct bpf_token *token)
 {
-	return call_int_hook(bpf_prog_load, prog, attr, token);
+	return call_int_hook(bpf_prog_load, prog, attr, uattr, token);
 }
 
 /**
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 7b867dfec88ba..aaf0a966880cf 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -6866,7 +6866,7 @@ static int selinux_ib_alloc_security(void *ib_sec)
 
 #ifdef CONFIG_BPF_SYSCALL
 static int selinux_bpf(int cmd, union bpf_attr *attr,
-				     unsigned int size)
+		       bpfptr_t uattr, unsigned int size)
 {
 	u32 sid = current_sid();
 	int ret;
@@ -6953,7 +6953,7 @@ static int selinux_bpf_prog(struct bpf_prog *prog)
 }
 
 static int selinux_bpf_map_create(struct bpf_map *map, union bpf_attr *attr,
-				  struct bpf_token *token)
+				  bpfptr_t uattr, struct bpf_token *token)
 {
 	struct bpf_security_struct *bpfsec;
 
@@ -6976,7 +6976,7 @@ static void selinux_bpf_map_free(struct bpf_map *map)
 }
 
 static int selinux_bpf_prog_load(struct bpf_prog *prog, union bpf_attr *attr,
-				 struct bpf_token *token)
+				 bpfptr_t uattr, struct bpf_token *token)
 {
 	struct bpf_security_struct *bpfsec;
 
-- 
2.48.1

Re: [PATCH 1/1] security: Propagate universal pointer data in bpf hooks

[Song Liu]

On Tue, Feb 25, 2025 at 4:31 PM Blaise Boscaccy
<bboscaccy@linux.microsoft.com> wrote:
>
> Certain bpf syscall subcommands are available for usage from both
> userspace and the kernel. LSM modules or eBPF gatekeeper programs may
> need to take a different course of action depending on whether or not
> a BPF syscall originated from the kernel or userspace.
>
> Additionally, some of the bpf_attr struct fields contain pointers to
> arbitrary memory. Currently the functionality to determine whether or
> not a pointer refers to kernel memory or userspace memory is exposed
> to the bpf verifier, but that information is missing from various LSM
> hooks.
>
> Here we augment the LSM hooks to provide this data, by simply passing
> the corresponding universal pointer in any hook that contains already
> contains a bpf_attr struct that corresponds to a subcommand that may
> be called from the kernel.

I think this information is useful for LSM hooks.

Question: Do we need a full bpfptr_t for these hooks, or just a boolean
"is_kernel or not"?

Thanks,
Song

> Signed-off-by: Blaise Boscaccy <bboscaccy@linux.microsoft.com>

Re: [PATCH 1/1] security: Propagate universal pointer data in bpf hooks

[Paul Moore]

On Wed, Feb 26, 2025 at 2:06 AM Song Liu <song@kernel.org> wrote:
> On Tue, Feb 25, 2025 at 4:31 PM Blaise Boscaccy
> <bboscaccy@linux.microsoft.com> wrote:
> >
> > Certain bpf syscall subcommands are available for usage from both
> > userspace and the kernel. LSM modules or eBPF gatekeeper programs may
> > need to take a different course of action depending on whether or not
> > a BPF syscall originated from the kernel or userspace.
> >
> > Additionally, some of the bpf_attr struct fields contain pointers to
> > arbitrary memory. Currently the functionality to determine whether or
> > not a pointer refers to kernel memory or userspace memory is exposed
> > to the bpf verifier, but that information is missing from various LSM
> > hooks.
> >
> > Here we augment the LSM hooks to provide this data, by simply passing
> > the corresponding universal pointer in any hook that contains already
> > contains a bpf_attr struct that corresponds to a subcommand that may
> > be called from the kernel.
>
> I think this information is useful for LSM hooks.

I've only looked at it quickly, but so far it seems reasonable.  I'm
going to take a closer look today.

> Question: Do we need a full bpfptr_t for these hooks, or just a boolean
> "is_kernel or not"?

I may be misunderstanding the patch, but what if we swapped the
existing 'union bpf_attr' parameter for a 'bpfptr_t' parameter?  That
would allow for both kernel and usermode pointers, complete with a
'is_kernel' flag; or am I missing something (likely)?

-- 
paul-moore.com

Re: [PATCH 1/1] security: Propagate universal pointer data in bpf hooks

[Blaise Boscaccy]

Paul Moore <paul@paul-moore.com> writes:

> On Wed, Feb 26, 2025 at 2:06 AM Song Liu <song@kernel.org> wrote:
>> On Tue, Feb 25, 2025 at 4:31 PM Blaise Boscaccy
>> <bboscaccy@linux.microsoft.com> wrote:
>> >
>> > Certain bpf syscall subcommands are available for usage from both
>> > userspace and the kernel. LSM modules or eBPF gatekeeper programs may
>> > need to take a different course of action depending on whether or not
>> > a BPF syscall originated from the kernel or userspace.
>> >
>> > Additionally, some of the bpf_attr struct fields contain pointers to
>> > arbitrary memory. Currently the functionality to determine whether or
>> > not a pointer refers to kernel memory or userspace memory is exposed
>> > to the bpf verifier, but that information is missing from various LSM
>> > hooks.
>> >
>> > Here we augment the LSM hooks to provide this data, by simply passing
>> > the corresponding universal pointer in any hook that contains already
>> > contains a bpf_attr struct that corresponds to a subcommand that may
>> > be called from the kernel.
>>
>> I think this information is useful for LSM hooks.
>
> I've only looked at it quickly, but so far it seems reasonable.  I'm
> going to take a closer look today.
>
>> Question: Do we need a full bpfptr_t for these hooks, or just a boolean
>> "is_kernel or not"?
>
> I may be misunderstanding the patch, but what if we swapped the
> existing 'union bpf_attr' parameter for a 'bpfptr_t' parameter?  That
> would allow for both kernel and usermode pointers, complete with a
> 'is_kernel' flag; or am I missing something (likely)?
>
> -- 
> paul-moore.com

bpfptr_t is just a typedef for a sockptr_t, which contains a void
pointer and bool, so if we replaced bpf_attr with it, we might lose a
bit of type safety going that route.

In syscall.c a most of the subcommand handlers have a

static int bpf_foo(union bpf_attr *attr, bpfptr_t uattr);

pattern that is used. I was trying to mimic for this patch.

The actual parts where the is_kernel flag gets used currently, is for
pointer chasing/copy stuff, e.g.

make_bpfptr(attr->insns, uattr.is_kernel)
make_bpfptr(attr->license, uattr.is_kernel)
make_bpfptr(attr->fd_array, uattr.is_kernel)

and subcommand structs may contain multiple pointers.

-blaise

Re: [PATCH 1/1] security: Propagate universal pointer data in bpf hooks

[Alexei Starovoitov]

On Tue, Feb 25, 2025 at 11:06 PM Song Liu <song@kernel.org> wrote:
>
> On Tue, Feb 25, 2025 at 4:31 PM Blaise Boscaccy
> <bboscaccy@linux.microsoft.com> wrote:
> >
> > Certain bpf syscall subcommands are available for usage from both
> > userspace and the kernel. LSM modules or eBPF gatekeeper programs may
> > need to take a different course of action depending on whether or not
> > a BPF syscall originated from the kernel or userspace.
> >
> > Additionally, some of the bpf_attr struct fields contain pointers to
> > arbitrary memory. Currently the functionality to determine whether or
> > not a pointer refers to kernel memory or userspace memory is exposed
> > to the bpf verifier, but that information is missing from various LSM
> > hooks.
> >
> > Here we augment the LSM hooks to provide this data, by simply passing
> > the corresponding universal pointer in any hook that contains already
> > contains a bpf_attr struct that corresponds to a subcommand that may
> > be called from the kernel.
>
> I think this information is useful for LSM hooks.
>
> Question: Do we need a full bpfptr_t for these hooks, or just a boolean
> "is_kernel or not"?

+1
Just passing the bool should do.
Passing uattr is a footgun. Last thing we need is to open up TOCTOU concerns.

Re: [PATCH 1/1] security: Propagate universal pointer data in bpf hooks

[Blaise Boscaccy]

Alexei Starovoitov <alexei.starovoitov@gmail.com> writes:

> On Tue, Feb 25, 2025 at 11:06 PM Song Liu <song@kernel.org> wrote:
>>
>> On Tue, Feb 25, 2025 at 4:31 PM Blaise Boscaccy
>> <bboscaccy@linux.microsoft.com> wrote:
>> >
>> > Certain bpf syscall subcommands are available for usage from both
>> > userspace and the kernel. LSM modules or eBPF gatekeeper programs may
>> > need to take a different course of action depending on whether or not
>> > a BPF syscall originated from the kernel or userspace.
>> >
>> > Additionally, some of the bpf_attr struct fields contain pointers to
>> > arbitrary memory. Currently the functionality to determine whether or
>> > not a pointer refers to kernel memory or userspace memory is exposed
>> > to the bpf verifier, but that information is missing from various LSM
>> > hooks.
>> >
>> > Here we augment the LSM hooks to provide this data, by simply passing
>> > the corresponding universal pointer in any hook that contains already
>> > contains a bpf_attr struct that corresponds to a subcommand that may
>> > be called from the kernel.
>>
>> I think this information is useful for LSM hooks.
>>
>> Question: Do we need a full bpfptr_t for these hooks, or just a boolean
>> "is_kernel or not"?
>
> +1
> Just passing the bool should do.
> Passing uattr is a footgun. Last thing we need is to open up TOCTOU concerns.

Sounds good to me, I'll rework it to use a bool instead.

-blaise

Re: [PATCH 1/1] security: Propagate universal pointer data in bpf hooks

[Song Liu]

On Wed, Feb 26, 2025 at 8:00 AM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Tue, Feb 25, 2025 at 11:06 PM Song Liu <song@kernel.org> wrote:
> >
> > On Tue, Feb 25, 2025 at 4:31 PM Blaise Boscaccy
> > <bboscaccy@linux.microsoft.com> wrote:
> > >
> > > Certain bpf syscall subcommands are available for usage from both
> > > userspace and the kernel. LSM modules or eBPF gatekeeper programs may
> > > need to take a different course of action depending on whether or not
> > > a BPF syscall originated from the kernel or userspace.
> > >
> > > Additionally, some of the bpf_attr struct fields contain pointers to
> > > arbitrary memory. Currently the functionality to determine whether or
> > > not a pointer refers to kernel memory or userspace memory is exposed
> > > to the bpf verifier, but that information is missing from various LSM
> > > hooks.
> > >
> > > Here we augment the LSM hooks to provide this data, by simply passing
> > > the corresponding universal pointer in any hook that contains already
> > > contains a bpf_attr struct that corresponds to a subcommand that may
> > > be called from the kernel.
> >
> > I think this information is useful for LSM hooks.
> >
> > Question: Do we need a full bpfptr_t for these hooks, or just a boolean
> > "is_kernel or not"?
>
> +1
> Just passing the bool should do.
> Passing uattr is a footgun. Last thing we need is to open up TOCTOU concerns.

Shall we also replace uattr with bool is_kernel in verifier.c? It appears to be
a good cleanup.

Thanks,
Song