linux-kernel - Re: [PATCH] serial: core: fix suspicious security_locked

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 7 May 2021 14:58:23 +0200
From:   Ondrej Mosnacek <omosnace@...hat.com>
To:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:     linux-serial@...r.kernel.org, David Howells <dhowells@...hat.com>,
        Matthew Garrett <mjg59@...gle.com>,
        Kees Cook <keescook@...omium.org>,
        Jiri Slaby <jirislaby@...nel.org>,
        SElinux list <selinux@...r.kernel.org>,
        Linux Security Module list 
        <linux-security-module@...r.kernel.org>,
        James Morris <jmorris@...ei.org>,
        Linux FS Devel <linux-fsdevel@...r.kernel.org>,
        Linux kernel mailing list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] serial: core: fix suspicious security_locked_down() call

On Fri, May 7, 2021 at 2:27 PM Greg Kroah-Hartman
<gregkh@...uxfoundation.org> wrote:
> On Fri, May 07, 2021 at 01:57:19PM +0200, Ondrej Mosnacek wrote:
> > The commit that added this check did so in a very strange way - first
> > security_locked_down() is called, its value stored into retval, and if
> > it's nonzero, then an additional check is made for (change_irq ||
> > change_port), and if this is true, the function returns. However, if
> > the goto exit branch is not taken, the code keeps the retval value and
> > continues executing the function. Then, depending on whether
> > uport->ops->verify_port is set, the retval value may or may not be reset
> > to zero and eventually the error value from security_locked_down() may
> > abort the function a few lines below.
> >
> > I will go out on a limb and assume that this isn't the intended behavior
> > and that an error value from security_locked_down() was supposed to
> > abort the function only in case (change_irq || change_port) is true.
>
> Are you _sure_ about this?
>
> Verification from the authors and users of this odd feature might be
> good to have, as I am loath to change how this works without them
> weighing in here.

I'm not completely sure and I'm with you on not merging this without
feedback from people involved in the original patch and/or whoever
understands the logic in said function.

-- 
Ondrej Mosnacek
Software Engineer, Linux Security - SELinux kernel
Red Hat, Inc.