Re: [PATCH] apparmor: Restore Y/N in /sys for apparmor's "enabled"

[John Johansen <john.johansen@canonical.com>]

On 4/8/19 10:25 AM, Kees Cook wrote:
> 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.)
> 

Right, though the legacy case that most document reference is apparmor=0/1
which is handled by __apparmor_enabled_setup()

still best not to break apparmor.enabled

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

redundant is fine, it will help protect against shooting ourselves in
the foot if some one ever tries changing the 0444

you can have my Acked-by: John Johansen <john.johansen@canonical.com>