On 4/8/19 10:25 AM, Kees Cook wrote:
> On Mon, Apr 8, 2019 at 9:58 AM John Johansen
> <john.johan...@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.johan...@canonical.com>
