Received-SPF: pass (google.com: domain of linux-kernel+bounces-19850-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20231213143813.6818-1-michael.weiss@aisec.fraunhofer.de>
 <20231213143813.6818-4-michael.weiss@aisec.fraunhofer.de> <20231215-golfanlage-beirren-f304f9dafaca@brauner>
 <61b39199-022d-4fd8-a7bf-158ee37b3c08@aisec.fraunhofer.de>
 <20231215-kubikmeter-aufsagen-62bf8d4e3d75@brauner> <CAADnVQKeUmV88OfQOfiX04HjKbXq7Wfcv+N3O=5kdL4vic6qrw@mail.gmail.com>
 <20231216-vorrecht-anrief-b096fa50b3f7@brauner> <CAADnVQK7MDUZTUxcqCH=unrrGExCjaagfJFqFPhVSLUisJVk_Q@mail.gmail.com>
 <20231218-chipsatz-abfangen-d62626dfb9e2@brauner> <CAHC9VhSZDMWJ_kh+RaB6dsPLQjkrjDY4bVkqsFDG3JtjinT_bQ@mail.gmail.com>
 <f38ceaaf-916a-4e44-9312-344ed1b4c9c4@aisec.fraunhofer.de>
 <CAHC9VhT3dbFc4DWc8WFRavWY1M+_+DzPbHuQ=PumROsx0rY2vA@mail.gmail.com> <6c2eb494-0cbf-4493-ae31-6dea519c4715@aisec.fraunhofer.de>
In-Reply-To: <6c2eb494-0cbf-4493-ae31-6dea519c4715@aisec.fraunhofer.de>
From: Paul Moore <paul@paul-moore.com>
Date: Mon, 8 Jan 2024 11:34:15 -0500
Message-ID: <CAHC9VhRcRT9DKtxmtamBCRvNF+dWW4MGx2KtS3CA3xcCGZf+ww@mail.gmail.com>
Subject: Re: [RFC PATCH v3 3/3] devguard: added device guard for mknod in
 non-initial userns
To: =?UTF-8?Q?Michael_Wei=C3=9F?= <michael.weiss@aisec.fraunhofer.de>
Cc: Christian Brauner <brauner@kernel.org>, Alexei Starovoitov <alexei.starovoitov@gmail.com>, 
	Alexander Mikhalitsyn <alexander@mihalicyn.com>, Alexei Starovoitov <ast@kernel.org>, 
	Daniel Borkmann <daniel@iogearbox.net>, Andrii Nakryiko <andrii@kernel.org>, 
	Martin KaFai Lau <martin.lau@linux.dev>, Song Liu <song@kernel.org>, Yonghong Song <yhs@fb.com>, 
	John Fastabend <john.fastabend@gmail.com>, KP Singh <kpsingh@kernel.org>, 
	Stanislav Fomichev <sdf@google.com>, Hao Luo <haoluo@google.com>, Jiri Olsa <jolsa@kernel.org>, 
	Quentin Monnet <quentin@isovalent.com>, Alexander Viro <viro@zeniv.linux.org.uk>, 
	Miklos Szeredi <miklos@szeredi.hu>, Amir Goldstein <amir73il@gmail.com>, 
	"Serge E. Hallyn" <serge@hallyn.com>, bpf <bpf@vger.kernel.org>, 
	LKML <linux-kernel@vger.kernel.org>, 
	Linux-Fsdevel <linux-fsdevel@vger.kernel.org>, 
	LSM List <linux-security-module@vger.kernel.org>, gyroidos@aisec.fraunhofer.de
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Mon, Jan 8, 2024 at 8:45=E2=80=AFAM Michael Wei=C3=9F
<michael.weiss@aisec.fraunhofer.de> wrote:
> On 29.12.23 23:31, Paul Moore wrote:
> > On Wed, Dec 27, 2023 at 9:31=E2=80=AFAM Michael Wei=C3=9F
> > <michael.weiss@aisec.fraunhofer.de> wrote:
> >> Hi Paul, what would you think about if we do it as shown in the
> >> patch below (untested)?
> >>
> >> I have adapted Christians patch slightly in a way that we do let
> >> all LSMs agree on if device access management should be done or not.
> >> Similar to the security_task_prctl() hook.
> >
> > I think it's worth taking a minute to talk about this proposed change
> > and the existing security_task_prctl() hook, as there is an important
> > difference between the two which is the source of my concern.
> >
> > If you look at the prctl() syscall implementation, right at the top of
> > the function you see the LSM hook:
> >
> >   SYSCALL_DEFINE(prctl, ...)
> >   {
> >     ...
> >
> >     error =3D security_task_prctl(...);
> >     if (error !=3D -ENOSYS)
> >       return error;
> >
> >     error =3D 0;
> >
> >     ....
> >   }
> >
> > While it is true that the LSM hook returns a "special" value, -ENOSYS,
> > from a practical perspective this is not significantly different from
> > the much more common zero value used to indicate no restriction from
> > the LSM layer.  However, the more important thing to note is that the
> > return value from security_task_prctl() does not influence any other
> > access controls in the caller outside of those implemented inside the
> > LSM; in fact the error code is reset to zero immediately after the LSM
> > hook.
> >
> > More on this below ...
> >
> >> diff --git a/fs/super.c b/fs/super.c
> >> index 076392396e72..6510168d51ce 100644
> >> --- a/fs/super.c
> >> +++ b/fs/super.c
> >> @@ -325,7 +325,7 @@ static struct super_block *alloc_super(struct file=
_system_type *type, int flags,
> >>  {
> >>         struct super_block *s =3D kzalloc(sizeof(struct super_block), =
 GFP_USER);
> >>         static const struct super_operations default_op;
> >> -       int i;
> >> +       int i, err;
> >>
> >>         if (!s)
> >>                 return NULL;
> >> @@ -362,8 +362,16 @@ static struct super_block *alloc_super(struct fil=
e_system_type *type, int flags,
> >>         }
> >>         s->s_bdi =3D &noop_backing_dev_info;
> >>         s->s_flags =3D flags;
> >> -       if (s->s_user_ns !=3D &init_user_ns)
> >> +
> >> +       err =3D security_sb_device_access(s);
> >> +       if (err < 0 && err !=3D -EOPNOTSUPP)
> >> +               goto fail;
> >> +
> >> +       if (err && s->s_user_ns !=3D &init_user_ns)
> >>                 s->s_iflags |=3D SB_I_NODEV;
> >> +       else
> >> +               s->s_iflags |=3D SB_I_MANAGED_DEVICES;
> >
> > This is my concern, depending on what the LSM hook returns, the
> > superblock's flags are set differently, affecting much more than just
> > a LSM-based security mechanism.
> >
> > LSMs should not be able to undermine, shortcut, or otherwise bypass
> > access controls built into other parts of the kernel.  In other words,
> > a LSM should only ever be able to deny an operation, it should not be
> > able to permit an operation that otherwise would have been denied.
>
> Hmm, OK. Then I can't see to come here any further as we would directly
> or indirectly set the superblock flags based on if a security hook is
> implemented or not, which I understand now is against LSM architecture.
> Thanks Paul for clarification.

No worries, thank you for posting to the LSM list for review and
consideration.  While it may take me a while to review something
(there always appears to be a backlog), I'm always happy to review
patches in this area and work with folks to find a solution.

--=20
paul-moore.com