diff mbox series

[bpf-next,v6,3/8] bpf: lsm: provide attachment points for BPF LSM programs

Message ID 20200325152629.6904-4-kpsingh@chromium.org (mailing list archive)
State New, archived
Headers show
Series MAC and Audit policy using eBPF (KRSI) | expand

Commit Message

KP Singh March 25, 2020, 3:26 p.m. UTC 
From: KP Singh <kpsingh@google.com>

When CONFIG_BPF_LSM is enabled, nop functions, bpf_lsm_<hook_name>, are
generated for each LSM hook. These functions are initialized as LSM
hooks in a subsequent patch.

Signed-off-by: KP Singh <kpsingh@google.com>
Reviewed-by: Brendan Jackman <jackmanb@google.com>
Reviewed-by: Florent Revest <revest@google.com>
Reviewed-by: Kees Cook <keescook@chromium.org>
Acked-by: Yonghong Song <yhs@fb.com>
---
 include/linux/bpf_lsm.h | 22 ++++++++++++++++++++++
 kernel/bpf/bpf_lsm.c    | 14 ++++++++++++++
 2 files changed, 36 insertions(+)
 create mode 100644 include/linux/bpf_lsm.h

Comments

Kees Cook March 25, 2020, 7:28 p.m. UTC | #1 
On Wed, Mar 25, 2020 at 04:26:24PM +0100, KP Singh wrote:
> From: KP Singh <kpsingh@google.com>
> 
> When CONFIG_BPF_LSM is enabled, nop functions, bpf_lsm_<hook_name>, are
> generated for each LSM hook. These functions are initialized as LSM
> hooks in a subsequent patch.
> 
> Signed-off-by: KP Singh <kpsingh@google.com>
> Reviewed-by: Brendan Jackman <jackmanb@google.com>
> Reviewed-by: Florent Revest <revest@google.com>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> Acked-by: Yonghong Song <yhs@fb.com>
> ---
>  include/linux/bpf_lsm.h | 22 ++++++++++++++++++++++
>  kernel/bpf/bpf_lsm.c    | 14 ++++++++++++++
>  2 files changed, 36 insertions(+)
>  create mode 100644 include/linux/bpf_lsm.h
> 
> diff --git a/include/linux/bpf_lsm.h b/include/linux/bpf_lsm.h
> new file mode 100644
> index 000000000000..83b96895829f
> --- /dev/null
> +++ b/include/linux/bpf_lsm.h
> @@ -0,0 +1,22 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +/*
> + * Copyright (C) 2020 Google LLC.
> + */
> +
> +#ifndef _LINUX_BPF_LSM_H
> +#define _LINUX_BPF_LSM_H
> +
> +#include <linux/bpf.h>
> +#include <linux/lsm_hooks.h>
> +
> +#ifdef CONFIG_BPF_LSM
> +
> +#define LSM_HOOK(RET, DEFAULT, NAME, ...) \
> +	RET bpf_lsm_##NAME(__VA_ARGS__);
> +#include <linux/lsm_hook_defs.h>
> +#undef LSM_HOOK
> +
> +#endif /* CONFIG_BPF_LSM */
> +
> +#endif /* _LINUX_BPF_LSM_H */
> diff --git a/kernel/bpf/bpf_lsm.c b/kernel/bpf/bpf_lsm.c
> index 82875039ca90..1210a819ca52 100644
> --- a/kernel/bpf/bpf_lsm.c
> +++ b/kernel/bpf/bpf_lsm.c
> @@ -7,6 +7,20 @@
>  #include <linux/filter.h>
>  #include <linux/bpf.h>
>  #include <linux/btf.h>
> +#include <linux/lsm_hooks.h>
> +#include <linux/bpf_lsm.h>
> +
> +/* For every LSM hook that allows attachment of BPF programs, declare a nop
> + * function where a BPF program can be attached.
> + */
> +#define LSM_HOOK(RET, DEFAULT, NAME, ...) 	\
> +noinline __weak RET bpf_lsm_##NAME(__VA_ARGS__)	\

I don't think the __weak is needed any more here?

> +{						\
> +	return DEFAULT;				\

I'm impressed that LSM_RET_VOID actually works. :)

-Kees

> +}
> +
> +#include <linux/lsm_hook_defs.h>
> +#undef LSM_HOOK
>  
>  const struct bpf_prog_ops lsm_prog_ops = {
>  };
> -- 
> 2.20.1
>
KP Singh March 25, 2020, 7:39 p.m. UTC | #2 
On 25-Mär 12:28, Kees Cook wrote:
> On Wed, Mar 25, 2020 at 04:26:24PM +0100, KP Singh wrote:
> > From: KP Singh <kpsingh@google.com>
> > 
> > When CONFIG_BPF_LSM is enabled, nop functions, bpf_lsm_<hook_name>, are
> > generated for each LSM hook. These functions are initialized as LSM
> > hooks in a subsequent patch.
> > 
> > Signed-off-by: KP Singh <kpsingh@google.com>
> > Reviewed-by: Brendan Jackman <jackmanb@google.com>
> > Reviewed-by: Florent Revest <revest@google.com>
> > Reviewed-by: Kees Cook <keescook@chromium.org>
> > Acked-by: Yonghong Song <yhs@fb.com>
> > ---
> >  include/linux/bpf_lsm.h | 22 ++++++++++++++++++++++
> >  kernel/bpf/bpf_lsm.c    | 14 ++++++++++++++
> >  2 files changed, 36 insertions(+)
> >  create mode 100644 include/linux/bpf_lsm.h
> > 
> > diff --git a/include/linux/bpf_lsm.h b/include/linux/bpf_lsm.h
> > new file mode 100644
> > index 000000000000..83b96895829f
> > --- /dev/null
> > +++ b/include/linux/bpf_lsm.h
> > @@ -0,0 +1,22 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +
> > +/*
> > + * Copyright (C) 2020 Google LLC.
> > + */
> > +
> > +#ifndef _LINUX_BPF_LSM_H
> > +#define _LINUX_BPF_LSM_H
> > +
> > +#include <linux/bpf.h>
> > +#include <linux/lsm_hooks.h>
> > +
> > +#ifdef CONFIG_BPF_LSM
> > +
> > +#define LSM_HOOK(RET, DEFAULT, NAME, ...) \
> > +	RET bpf_lsm_##NAME(__VA_ARGS__);
> > +#include <linux/lsm_hook_defs.h>
> > +#undef LSM_HOOK
> > +
> > +#endif /* CONFIG_BPF_LSM */
> > +
> > +#endif /* _LINUX_BPF_LSM_H */
> > diff --git a/kernel/bpf/bpf_lsm.c b/kernel/bpf/bpf_lsm.c
> > index 82875039ca90..1210a819ca52 100644
> > --- a/kernel/bpf/bpf_lsm.c
> > +++ b/kernel/bpf/bpf_lsm.c
> > @@ -7,6 +7,20 @@
> >  #include <linux/filter.h>
> >  #include <linux/bpf.h>
> >  #include <linux/btf.h>
> > +#include <linux/lsm_hooks.h>
> > +#include <linux/bpf_lsm.h>
> > +
> > +/* For every LSM hook that allows attachment of BPF programs, declare a nop
> > + * function where a BPF program can be attached.
> > + */
> > +#define LSM_HOOK(RET, DEFAULT, NAME, ...) 	\
> > +noinline __weak RET bpf_lsm_##NAME(__VA_ARGS__)	\
> 
> I don't think the __weak is needed any more here?

This was suggested in:

 https://lore.kernel.org/bpf/20200221022537.wbmhdfkdbfvw2pww@ast-mbp/

"I think I saw cases when gcc ignored 'noinline' when function is
defined in the same file and still performed inlining while keeping
the function body.  To be safe I think __weak is necessary. That will
guarantee noinline."

It happened to work nicely with the previous approach for the special
hooks but the actual reason for adding the __weak was to guarrantee
that these functions don't get inlined.

> 
> > +{						\
> > +	return DEFAULT;				\
> 
> I'm impressed that LSM_RET_VOID actually works. :)

All the credit goes to Andrii :)

- KP

> 
> -Kees
> 
> > +}
> > +
> > +#include <linux/lsm_hook_defs.h>
> > +#undef LSM_HOOK
> >  
> >  const struct bpf_prog_ops lsm_prog_ops = {
> >  };
> > -- 
> > 2.20.1
> > 
> 
> -- 
> Kees Cook
Kees Cook March 25, 2020, 8:07 p.m. UTC | #3 
On Wed, Mar 25, 2020 at 08:39:56PM +0100, KP Singh wrote:
> On 25-Mär 12:28, Kees Cook wrote:
> > On Wed, Mar 25, 2020 at 04:26:24PM +0100, KP Singh wrote:
> > > +noinline __weak RET bpf_lsm_##NAME(__VA_ARGS__)	\
> > 
> > I don't think the __weak is needed any more here?
> 
> This was suggested in:
> 
>  https://lore.kernel.org/bpf/20200221022537.wbmhdfkdbfvw2pww@ast-mbp/
> 
> "I think I saw cases when gcc ignored 'noinline' when function is
> defined in the same file and still performed inlining while keeping
> the function body.  To be safe I think __weak is necessary. That will
> guarantee noinline."
> 
> It happened to work nicely with the previous approach for the special
> hooks but the actual reason for adding the __weak was to guarrantee
> that these functions don't get inlined.

Oh, hrm. Well, okay. That rationale would imply that the "noinline"
macro needs adjustment instead, but that can be separate, something like:

include/linux/compiler_attributes.h

-#define noinline __attribute__((__noinline__))
+#define noinline __attribute__((__noinline__)) __attribute__((__weak__))

With a comment, etc...

-Kees

> 
> > 
> > > +{						\
> > > +	return DEFAULT;				\
> > 
> > I'm impressed that LSM_RET_VOID actually works. :)
> 
> All the credit goes to Andrii :)
> 
> - KP
> 
> > 
> > -Kees
> > 
> > > +}
> > > +
> > > +#include <linux/lsm_hook_defs.h>
> > > +#undef LSM_HOOK
> > >  
> > >  const struct bpf_prog_ops lsm_prog_ops = {
> > >  };
> > > -- 
> > > 2.20.1
> > > 
> > 
> > -- 
> > Kees Cook
KP Singh March 25, 2020, 8:14 p.m. UTC | #4 
On 25-Mär 13:07, Kees Cook wrote:
> On Wed, Mar 25, 2020 at 08:39:56PM +0100, KP Singh wrote:
> > On 25-Mär 12:28, Kees Cook wrote:
> > > On Wed, Mar 25, 2020 at 04:26:24PM +0100, KP Singh wrote:
> > > > +noinline __weak RET bpf_lsm_##NAME(__VA_ARGS__)	\
> > > 
> > > I don't think the __weak is needed any more here?
> > 
> > This was suggested in:
> > 
> >  https://lore.kernel.org/bpf/20200221022537.wbmhdfkdbfvw2pww@ast-mbp/
> > 
> > "I think I saw cases when gcc ignored 'noinline' when function is
> > defined in the same file and still performed inlining while keeping
> > the function body.  To be safe I think __weak is necessary. That will
> > guarantee noinline."
> > 
> > It happened to work nicely with the previous approach for the special
> > hooks but the actual reason for adding the __weak was to guarrantee
> > that these functions don't get inlined.
> 
> Oh, hrm. Well, okay. That rationale would imply that the "noinline"
> macro needs adjustment instead, but that can be separate, something like:
> 
> include/linux/compiler_attributes.h
> 
> -#define noinline __attribute__((__noinline__))
> +#define noinline __attribute__((__noinline__)) __attribute__((__weak__))
> 
> With a comment, etc...

Sounds reasonable, I will drop the __weak from this and send a
separate patch for this.

- KP

> 
> -Kees
> 
> > 
> > > 
> > > > +{						\
> > > > +	return DEFAULT;				\
> > > 
> > > I'm impressed that LSM_RET_VOID actually works. :)
> > 
> > All the credit goes to Andrii :)
> > 
> > - KP
> > 
> > > 
> > > -Kees
> > > 
> > > > +}
> > > > +
> > > > +#include <linux/lsm_hook_defs.h>
> > > > +#undef LSM_HOOK
> > > >  
> > > >  const struct bpf_prog_ops lsm_prog_ops = {
> > > >  };
> > > > -- 
> > > > 2.20.1
> > > > 
> > > 
> > > -- 
> > > Kees Cook
> 
> -- 
> Kees Cook
diff mbox series

Patch

diff --git a/include/linux/bpf_lsm.h b/include/linux/bpf_lsm.h
new file mode 100644
index 000000000000..83b96895829f
--- /dev/null
+++ b/include/linux/bpf_lsm.h
@@ -0,0 +1,22 @@ 
+/* SPDX-License-Identifier: GPL-2.0 */
+
+/*
+ * Copyright (C) 2020 Google LLC.
+ */
+
+#ifndef _LINUX_BPF_LSM_H
+#define _LINUX_BPF_LSM_H
+
+#include <linux/bpf.h>
+#include <linux/lsm_hooks.h>
+
+#ifdef CONFIG_BPF_LSM
+
+#define LSM_HOOK(RET, DEFAULT, NAME, ...) \
+	RET bpf_lsm_##NAME(__VA_ARGS__);
+#include <linux/lsm_hook_defs.h>
+#undef LSM_HOOK
+
+#endif /* CONFIG_BPF_LSM */
+
+#endif /* _LINUX_BPF_LSM_H */
diff --git a/kernel/bpf/bpf_lsm.c b/kernel/bpf/bpf_lsm.c
index 82875039ca90..1210a819ca52 100644
--- a/kernel/bpf/bpf_lsm.c
+++ b/kernel/bpf/bpf_lsm.c
@@ -7,6 +7,20 @@ 
 #include <linux/filter.h>
 #include <linux/bpf.h>
 #include <linux/btf.h>
+#include <linux/lsm_hooks.h>
+#include <linux/bpf_lsm.h>
+
+/* For every LSM hook that allows attachment of BPF programs, declare a nop
+ * function where a BPF program can be attached.
+ */
+#define LSM_HOOK(RET, DEFAULT, NAME, ...) 	\
+noinline __weak RET bpf_lsm_##NAME(__VA_ARGS__)	\
+{						\
+	return DEFAULT;				\
+}
+
+#include <linux/lsm_hook_defs.h>
+#undef LSM_HOOK
 
 const struct bpf_prog_ops lsm_prog_ops = {
 };