From: Christian Brauner <christian.brauner@ubuntu.com>
To: Casey Schaufler <casey@schaufler-ca.com>
Cc: David Howells <dhowells@redhat.com>,
syzbot <syzbot+d1e3b1d92d25abf97943@syzkaller.appspotmail.com>,
andriin@fb.com, ast@kernel.org, bpf@vger.kernel.org,
daniel@iogearbox.net, dvyukov@google.com, jmorris@namei.org,
kafai@fb.com, kpsingh@google.com, linux-fsdevel@vger.kernel.org,
linux-kernel@vger.kernel.org,
linux-security-module@vger.kernel.org, netdev@vger.kernel.org,
paul@paul-moore.com, selinux@vger.kernel.org,
songliubraving@fb.com, stephen.smalley.work@gmail.com,
syzkaller-bugs@googlegroups.com, tonymarislogistics@yandex.com,
viro@zeniv.linux.org.uk, yhs@fb.com
Subject: Re: [syzbot] general protection fault in legacy_parse_param
Date: Tue, 31 Aug 2021 09:38:18 +0200 [thread overview]
Message-ID: <20210831073818.oojyjqyiogel7hll@wittgenstein> (raw)
In-Reply-To: <3354839e-5e7a-08c7-277a-9bbebfbfc0bc@schaufler-ca.com>
On Mon, Aug 30, 2021 at 10:41:29AM -0700, Casey Schaufler wrote:
> On 8/30/2021 9:57 AM, Christian Brauner wrote:
> > On Mon, Aug 30, 2021 at 09:40:57AM -0700, Casey Schaufler wrote:
> >> On 8/30/2021 7:25 AM, Casey Schaufler wrote:
> >>> On 8/30/2021 5:23 AM, Christian Brauner wrote:
> >>>> On Fri, Aug 27, 2021 at 07:11:18PM -0700, syzbot wrote:
> >>>>> syzbot has bisected this issue to:
> >>>>>
> >>>>> commit 54261af473be4c5481f6196064445d2945f2bdab
> >>>>> Author: KP Singh <kpsingh@google.com>
> >>>>> Date: Thu Apr 30 15:52:40 2020 +0000
> >>>>>
> >>>>> security: Fix the default value of fs_context_parse_param hook
> >>>>>
> >>>>> bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=160c5d75300000
> >>>>> start commit: 77dd11439b86 Merge tag 'drm-fixes-2021-08-27' of git://ano..
> >>>>> git tree: upstream
> >>>>> final oops: https://syzkaller.appspot.com/x/report.txt?x=150c5d75300000
> >>>>> console output: https://syzkaller.appspot.com/x/log.txt?x=110c5d75300000
> >>>>> kernel config: https://syzkaller.appspot.com/x/.config?x=2fd902af77ff1e56
> >>>>> dashboard link: https://syzkaller.appspot.com/bug?extid=d1e3b1d92d25abf97943
> >>>>> syz repro: https://syzkaller.appspot.com/x/repro.syz?x=126d084d300000
> >>>>> C reproducer: https://syzkaller.appspot.com/x/repro.c?x=16216eb1300000
> >>>>>
> >>>>> Reported-by: syzbot+d1e3b1d92d25abf97943@syzkaller.appspotmail.com
> >>>>> Fixes: 54261af473be ("security: Fix the default value of fs_context_parse_param hook")
> >>>>>
> >>>>> For information about bisection process see: https://goo.gl/tpsmEJ#bisection
> >>>> So ok, this seems somewhat clear now. When smack and
> >>>> CONFIG_BPF_LSM=y
> >>>> is selected the bpf LSM will register NOP handlers including
> >>>>
> >>>> bpf_lsm_fs_context_fs_param()
> >>>>
> >>>> for the
> >>>>
> >>>> fs_context_fs_param
> >>>>
> >>>> LSM hook. The bpf LSM runs last, i.e. after smack according to:
> >>>>
> >>>> CONFIG_LSM="landlock,lockdown,yama,safesetid,integrity,tomoyo,smack,bpf"
> >>>>
> >>>> in the appended config. The smack hook runs and sets
> >>>>
> >>>> param->string = NULL
> >>>>
> >>>> then the bpf NOP handler runs returning -ENOPARM indicating to the vfs
> >>>> parameter parser that this is not a security module option so it should
> >>>> proceed processing the parameter subsequently causing the crash because
> >>>> param->string is not allowed to be NULL (Which the vfs parameter parser
> >>>> verifies early in fsconfig().).
> >>> The security_fs_context_parse_param() function is incorrectly
> >>> implemented using the call_int_hook() macro. It should return
> >>> zero if any of the modules return zero. It does not follow the
> >>> usual failure model of LSM hooks. It could be argued that the
> >>> code was fine before the addition of the BPF hook, but it was
> >>> going to fail as soon as any two security modules provided
> >>> mount options.
> >>>
> >>> Regardless, I will have a patch later today. Thank you for
> >>> tracking this down.
> >> Here's my proposed patch. I'll tidy it up with a proper
> >> commit message if it looks alright to y'all. I've tested
> >> with Smack and with and without BPF.
> > Looks good to me.
> > On question, in contrast to smack, selinux returns 1 instead of 0 on
> > success. So selinux would cause an early return preventing other hooks
> > from running. Just making sure that this is intentional.
> >
> > Iirc, this would mean that selinux causes fsconfig() to return a
> > positive value to userspace which I think is a bug; likely in selinux.
> > So I think selinux should either return 0 or the security hook itself
> > needs to overwrite a positive value with a sensible errno that can be
> > seen by userspace.
>
> I think that I agree. The SELinux and Smack versions of the
> hook are almost identical except for setting rc to 1 in the
> SELinux case. And returning 1 makes no sense if you follow
> the callers back. David Howells wrote both the SELinux and
> Smack versions. David - why are they different? which is correct?
The documentation for fs_context_parse_param notes:
* @fs_context_parse_param:
* Userspace provided a parameter to configure a superblock. The LSM may
* reject it with an error and may use it for itself, in which case it
* should return 0; otherwise it should return -ENOPARAM to pass it on to
* the filesystem.
* @fc indicates the filesystem context.
* @param The parameter
So we should simply make selinux return 0 on top of your patch when it
has consumed the option.
prev parent reply other threads:[~2021-08-31 7:38 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-07-03 5:41 [syzbot] general protection fault in legacy_parse_param syzbot
2021-07-03 5:51 ` Dmitry Vyukov
2021-07-03 22:16 ` Casey Schaufler
2021-07-04 14:14 ` Paul Moore
2021-07-05 5:52 ` Dmitry Vyukov
2021-07-06 12:50 ` Paul Moore
2021-08-27 15:30 ` Christian Brauner
2021-08-27 15:40 ` Casey Schaufler
2021-08-27 16:26 ` Christian Brauner
2021-08-27 14:49 ` syzbot
2021-08-28 2:11 ` syzbot
2021-08-30 12:23 ` Christian Brauner
2021-08-30 14:25 ` Casey Schaufler
2021-08-30 16:40 ` Casey Schaufler
2021-08-30 16:57 ` Christian Brauner
2021-08-30 17:41 ` Casey Schaufler
2021-08-31 7:38 ` Christian Brauner [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20210831073818.oojyjqyiogel7hll@wittgenstein \
--to=christian.brauner@ubuntu.com \
--cc=andriin@fb.com \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=casey@schaufler-ca.com \
--cc=daniel@iogearbox.net \
--cc=dhowells@redhat.com \
--cc=dvyukov@google.com \
--cc=jmorris@namei.org \
--cc=kafai@fb.com \
--cc=kpsingh@google.com \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-security-module@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=paul@paul-moore.com \
--cc=selinux@vger.kernel.org \
--cc=songliubraving@fb.com \
--cc=stephen.smalley.work@gmail.com \
--cc=syzbot+d1e3b1d92d25abf97943@syzkaller.appspotmail.com \
--cc=syzkaller-bugs@googlegroups.com \
--cc=tonymarislogistics@yandex.com \
--cc=viro@zeniv.linux.org.uk \
--cc=yhs@fb.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).