diff mbox series

[bpf-next,v3,3/4] selftests/bpf: Add config dependency on BLK_DEV_LOOP

Message ID 20201203005807.486320-4-kpsingh@chromium.org (mailing list archive)
State Superseded
Delegated to: BPF
Headers show
Series Fixes for ima selftest | expand

Checks

Context Check Description
netdev/cover_letter success Link
netdev/fixes_present success Link
netdev/patch_count success Link
netdev/tree_selection success Clearly marked for bpf-next
netdev/subject_prefix success Link
netdev/source_inline success Was 0 now: 0
netdev/verify_signedoff success Link
netdev/module_param success Was 0 now: 0
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/verify_fixes success Link
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 4 lines checked
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/header_inline success Link
netdev/stable success Stable not CCed

Commit Message

KP Singh Dec. 3, 2020, 12:58 a.m. UTC 
From: KP Singh <kpsingh@google.com>

The ima selftest restricts its scope to a test filesystem image
mounted on a loop device and prevents permanent ima policy changes for
the whole system.

Fixes: 34b82d3ac105 ("bpf: Add a selftest for bpf_ima_inode_hash")
Reported-by: Andrii Nakryiko <andrii@kernel.org>
Signed-off-by: KP Singh <kpsingh@google.com>
---
 tools/testing/selftests/bpf/config | 1 +
 1 file changed, 1 insertion(+)

Comments

Andrii Nakryiko Dec. 3, 2020, 5:56 a.m. UTC | #1 
On Wed, Dec 2, 2020 at 4:58 PM KP Singh <kpsingh@chromium.org> wrote:
>
> From: KP Singh <kpsingh@google.com>
>
> The ima selftest restricts its scope to a test filesystem image
> mounted on a loop device and prevents permanent ima policy changes for
> the whole system.
>
> Fixes: 34b82d3ac105 ("bpf: Add a selftest for bpf_ima_inode_hash")
> Reported-by: Andrii Nakryiko <andrii@kernel.org>
> Signed-off-by: KP Singh <kpsingh@google.com>
> ---
>  tools/testing/selftests/bpf/config | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/tools/testing/selftests/bpf/config b/tools/testing/selftests/bpf/config
> index 365bf9771b07..37e1f303fc11 100644
> --- a/tools/testing/selftests/bpf/config
> +++ b/tools/testing/selftests/bpf/config
> @@ -43,3 +43,4 @@ CONFIG_IMA=y
>  CONFIG_SECURITYFS=y
>  CONFIG_IMA_WRITE_POLICY=y
>  CONFIG_IMA_READ_POLICY=y
> +CONFIG_BLK_DEV_LOOP=y
> --


You mentioned also that CONFIG_LSM="selinux,bpf,integrity" is needed,
no? Let's add that as well?

> 2.29.2.576.ga3fc446d84-goog
>
KP Singh Dec. 3, 2020, 10:56 a.m. UTC | #2 
On Thu, Dec 3, 2020 at 6:56 AM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Wed, Dec 2, 2020 at 4:58 PM KP Singh <kpsingh@chromium.org> wrote:
> >
> > From: KP Singh <kpsingh@google.com>
> >
> > The ima selftest restricts its scope to a test filesystem image
> > mounted on a loop device and prevents permanent ima policy changes for
> > the whole system.
> >
> > Fixes: 34b82d3ac105 ("bpf: Add a selftest for bpf_ima_inode_hash")
> > Reported-by: Andrii Nakryiko <andrii@kernel.org>
> > Signed-off-by: KP Singh <kpsingh@google.com>
> > ---
> >  tools/testing/selftests/bpf/config | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/tools/testing/selftests/bpf/config b/tools/testing/selftests/bpf/config
> > index 365bf9771b07..37e1f303fc11 100644
> > --- a/tools/testing/selftests/bpf/config
> > +++ b/tools/testing/selftests/bpf/config
> > @@ -43,3 +43,4 @@ CONFIG_IMA=y
> >  CONFIG_SECURITYFS=y
> >  CONFIG_IMA_WRITE_POLICY=y
> >  CONFIG_IMA_READ_POLICY=y
> > +CONFIG_BLK_DEV_LOOP=y
> > --
>
>
> You mentioned also that CONFIG_LSM="selinux,bpf,integrity" is needed,
> no? Let's add that as well?

I did not add it because we did not do it when we added "bpf" to the list and

I also don't think selinux is really required here which might be worse in
some cases (e.g. when the required config options for
SELinux are not selected).

Also, when one selects CONFIG_BPF_LSM or CONFIG_IMA from make
menuconfig / nconfig, we get "bpf" and "integrity" appended by default:

We can add a comment that says that says:

  "Please ensure "bpf" and "integrity" are present in CONFIG_LSM"

Now, I was not sure if adding a comment would break any scripts that people
have that parse this file, so I avoided it. But overriding the string
completely
might not be a good idea.

>
> > 2.29.2.576.ga3fc446d84-goog
> >
KP Singh Dec. 3, 2020, 5:35 p.m. UTC | #3 
On Thu, Dec 3, 2020 at 11:56 AM KP Singh <kpsingh@chromium.org> wrote:
>
> On Thu, Dec 3, 2020 at 6:56 AM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> >
> > On Wed, Dec 2, 2020 at 4:58 PM KP Singh <kpsingh@chromium.org> wrote:
> > >
> > > From: KP Singh <kpsingh@google.com>
> > >
> > > The ima selftest restricts its scope to a test filesystem image
> > > mounted on a loop device and prevents permanent ima policy changes for
> > > the whole system.
> > >
> > > Fixes: 34b82d3ac105 ("bpf: Add a selftest for bpf_ima_inode_hash")
> > > Reported-by: Andrii Nakryiko <andrii@kernel.org>
> > > Signed-off-by: KP Singh <kpsingh@google.com>
> > > ---
> > >  tools/testing/selftests/bpf/config | 1 +
> > >  1 file changed, 1 insertion(+)
> > >
> > > diff --git a/tools/testing/selftests/bpf/config b/tools/testing/selftests/bpf/config
> > > index 365bf9771b07..37e1f303fc11 100644
> > > --- a/tools/testing/selftests/bpf/config
> > > +++ b/tools/testing/selftests/bpf/config
> > > @@ -43,3 +43,4 @@ CONFIG_IMA=y
> > >  CONFIG_SECURITYFS=y
> > >  CONFIG_IMA_WRITE_POLICY=y
> > >  CONFIG_IMA_READ_POLICY=y
> > > +CONFIG_BLK_DEV_LOOP=y
> > > --
> >
> >
> > You mentioned also that CONFIG_LSM="selinux,bpf,integrity" is needed,
> > no? Let's add that as well?
>
> I did not add it because we did not do it when we added "bpf" to the list and
>
> I also don't think selinux is really required here which might be worse in
> some cases (e.g. when the required config options for
> SELinux are not selected).
>
> Also, when one selects CONFIG_BPF_LSM or CONFIG_IMA from make
> menuconfig / nconfig, we get "bpf" and "integrity" appended by default:
>
> We can add a comment that says that says:
>
>   "Please ensure "bpf" and "integrity" are present in CONFIG_LSM"
>
> Now, I was not sure if adding a comment would break any scripts that people
> have that parse this file, so I avoided it. But overriding the string
> completely
> might not be a good idea.

If it's okay, I can send the v4 out now and we can add the comment or CONFIG_LSM
in a separate patch?
Andrii Nakryiko Dec. 3, 2020, 6:06 p.m. UTC | #4 
On Thu, Dec 3, 2020 at 9:35 AM KP Singh <kpsingh@chromium.org> wrote:
>
> On Thu, Dec 3, 2020 at 11:56 AM KP Singh <kpsingh@chromium.org> wrote:
> >
> > On Thu, Dec 3, 2020 at 6:56 AM Andrii Nakryiko
> > <andrii.nakryiko@gmail.com> wrote:
> > >
> > > On Wed, Dec 2, 2020 at 4:58 PM KP Singh <kpsingh@chromium.org> wrote:
> > > >
> > > > From: KP Singh <kpsingh@google.com>
> > > >
> > > > The ima selftest restricts its scope to a test filesystem image
> > > > mounted on a loop device and prevents permanent ima policy changes for
> > > > the whole system.
> > > >
> > > > Fixes: 34b82d3ac105 ("bpf: Add a selftest for bpf_ima_inode_hash")
> > > > Reported-by: Andrii Nakryiko <andrii@kernel.org>
> > > > Signed-off-by: KP Singh <kpsingh@google.com>
> > > > ---
> > > >  tools/testing/selftests/bpf/config | 1 +
> > > >  1 file changed, 1 insertion(+)
> > > >
> > > > diff --git a/tools/testing/selftests/bpf/config b/tools/testing/selftests/bpf/config
> > > > index 365bf9771b07..37e1f303fc11 100644
> > > > --- a/tools/testing/selftests/bpf/config
> > > > +++ b/tools/testing/selftests/bpf/config
> > > > @@ -43,3 +43,4 @@ CONFIG_IMA=y
> > > >  CONFIG_SECURITYFS=y
> > > >  CONFIG_IMA_WRITE_POLICY=y
> > > >  CONFIG_IMA_READ_POLICY=y
> > > > +CONFIG_BLK_DEV_LOOP=y
> > > > --
> > >
> > >
> > > You mentioned also that CONFIG_LSM="selinux,bpf,integrity" is needed,
> > > no? Let's add that as well?
> >
> > I did not add it because we did not do it when we added "bpf" to the list and
> >
> > I also don't think selinux is really required here which might be worse in
> > some cases (e.g. when the required config options for
> > SELinux are not selected).
> >
> > Also, when one selects CONFIG_BPF_LSM or CONFIG_IMA from make
> > menuconfig / nconfig, we get "bpf" and "integrity" appended by default:
> >
> > We can add a comment that says that says:
> >
> >   "Please ensure "bpf" and "integrity" are present in CONFIG_LSM"
> >
> > Now, I was not sure if adding a comment would break any scripts that people

Any infra I'm in charge of is not using this config in an automated
fashion, so adding

# CONFIG_LSM="bpf,integrity"

would work just fine. But I can't know if anyone else relies on this.
But # comments are part of Kconfig "spec", so probably is ok to add.

> > have that parse this file, so I avoided it. But overriding the string
> > completely
> > might not be a good idea.
>
> If it's okay, I can send the v4 out now and we can add the comment or CONFIG_LSM
> in a separate patch?

Sure.
diff mbox series

Patch

diff --git a/tools/testing/selftests/bpf/config b/tools/testing/selftests/bpf/config
index 365bf9771b07..37e1f303fc11 100644
--- a/tools/testing/selftests/bpf/config
+++ b/tools/testing/selftests/bpf/config
@@ -43,3 +43,4 @@  CONFIG_IMA=y
 CONFIG_SECURITYFS=y
 CONFIG_IMA_WRITE_POLICY=y
 CONFIG_IMA_READ_POLICY=y
+CONFIG_BLK_DEV_LOOP=y