Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20190408160706.GA18786@beast> <73c80352-ded1-626b-0eb0-a9481165f25d@canonical.com>
In-Reply-To: <73c80352-ded1-626b-0eb0-a9481165f25d@canonical.com>
From:   Kees Cook <keescook@chromium.org>
Date:   Mon, 8 Apr 2019 10:25:28 -0700
Message-ID: <CAGXu5j+k86VtegoRxV1s++YZdMxXEhQhqx1Yh7eu+YKHn0wNZA@mail.gmail.com>
Subject: Re: [PATCH] apparmor: Restore Y/N in /sys for apparmor's "enabled"
To:     John Johansen <john.johansen@canonical.com>
Cc:     James Morris <jmorris@namei.org>,
        David Rheinsberg <david.rheinsberg@gmail.com>,
        "Serge E. Hallyn" <serge@hallyn.com>,
        linux-security-module <linux-security-module@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Mon, Apr 8, 2019 at 9:58 AM John Johansen
<john.johansen@canonical.com> wrote:
> > +/* Can only be set before AppArmor is initialized (i.e. on boot cmdline). */
> > +static int param_set_aaintbool(const char *val, const struct kernel_param *kp)
> > +{
> > +     struct kernel_param kp_local;
> > +     bool value;
> > +     int error;
> > +
> > +     if (apparmor_initialized)
> > +             return -EPERM;
> > +
> This isn't sufficient/correct. apparmor_initialized is only set after
> apparmor has gone through and completed initialization. However if
> apparmor is not selected as one of the LSMs to enable, then this check
> won't stop apparmor_enabled from being set post boot.
>
> However with the apparmor_enabled param being 0444 and the
> apparmor_enabled_setup() fn handling boot cmdline do with even need
> the set parameter fn?

Yup, that's true. I've gone and tested this, and yes, the 0444 is
sufficient to protect the logic here (even if root chmods the inode).
So the test here is redundant. However, very early in the threads
about LSM boot cmdline enabling it was made clear that
"apparmor.enabled=..." needed to stay working, which means the "set"
op is still needed. (But I'm happy to do whatever you want here -- I
was just trying to keep the functionality as it was.)

Should I send a v2 without the "initialized" check or is this okay to
leave as-is with the redundant check?

-- 
Kees Cook