Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) client-ip=2620:137:e000::3:7;
MIME-Version: 1.0
References: <20231006051801.423973-1-sumit.garg@linaro.org> <CAHk-=whOPoLaWM8S8GgoOPT7a2+nMH5h3TLKtn=R_3w4R1_Uvg@mail.gmail.com>
In-Reply-To: <CAHk-=whOPoLaWM8S8GgoOPT7a2+nMH5h3TLKtn=R_3w4R1_Uvg@mail.gmail.com>
From:   Sumit Garg <sumit.garg@linaro.org>
Date:   Wed, 11 Oct 2023 11:22:00 +0530
Message-ID: <CAFA6WYMsG8e2TCCT39HJ6SE1YKuK0sJRyoyyCYmujh_C_QFfGA@mail.gmail.com>
Subject: Re: [PATCH v2] KEYS: trusted: Remove redundant static calls usage
To:     Linus Torvalds <torvalds@linux-foundation.org>
Cc:     David Howells <dhowells@redhat.com>, jarkko@kernel.org,
        peterz@infradead.org, zohar@linux.ibm.com,
        linux-kernel@vger.kernel.org, linux-integrity@vger.kernel.org,
        jejb@linux.ibm.com, David.Kaplan@amd.com, bp@alien8.de,
        mingo@kernel.org, x86@kernel.org, regressions@leemhuis.info,
        Hyeonggon Yoo <42.hyeyoo@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, 10 Oct 2023 at 23:59, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Thu, 5 Oct 2023 at 22:18, Sumit Garg <sumit.garg@linaro.org> wrote:
> >
> > Static calls invocations aren't well supported from module __init and
> > __exit functions. Especially the static call from cleanup_trusted() led
> > to a crash on x86 kernel with CONFIG_DEBUG_VIRTUAL=y.
> >
> > However, the usage of static call invocations for trusted_key_init()
> > and trusted_key_exit() don't add any value from either a performance or
> > security perspective. Hence switch to use indirect function calls instead.
>
> I applied this patch to my tree, since it is a fix for the issue, and
> doesn't change any logic otherwise.

Thanks.

>
> However, I do note that the code logic is completely broken. It was
> broken before too, and apparently causes no problems, but it's still
> wrong.
>
> That's a separate issue, and would want a separate patch, but since I
> noticed it when applying this one, I'm replying here:
>
> > +               trusted_key_exit = trusted_key_sources[i].ops->exit;
> >                 migratable = trusted_key_sources[i].ops->migratable;
> >
> > -               ret = static_call(trusted_key_init)();
> > +               ret = trusted_key_sources[i].ops->init();
> >                 if (!ret)
> >                         break;
>
> Note how this sets "trusted_key_exit" even when the ->init() function fails.
>
> Then we potentially do the module exit:
>
> >  static void __exit cleanup_trusted(void)
> >  {
> > -       static_call_cond(trusted_key_exit)();
> > +       if (trusted_key_exit)
> > +               (*trusted_key_exit)();
> >  }
>
> With an exit function that doesn't match a successful init() call.
>
> Now, *normally* this isn't a problem, because if the init() call
> fails, we'll go on to the next one, and if they *all* fail, we'll fail
> the module load, and we obviously won't call the cleanup_trusted()
> function at all.
>
> EXCEPT.
>
> We have this:
>
>         /*
>          * encrypted_keys.ko depends on successful load of this module even if
>          * trusted key implementation is not found.
>          */
>         if (ret == -ENODEV)
>                 return 0;
>
> so that init() may actually have failed, and we still succeed in
> loading the module, and now we will call that exit function to clean
> up something that was never successfully done.

Here we consider -ENODEV as a success case since we don't want to
block encrypted keys module loading since it can use user key as
master key instead.

>
> This hopefully doesn't matter in practice, and the cleanup function
> will just not do anything, but it is illogical and inconsistent. So I
> think it should be fixed.

Agree as the exit function won't do anything without the device being
present but we should make it consistent.

-Sumit

> But as mentioned, this is a separate issue
> from the whole "you currently can't do static calls from __exit
> functions" issue.
>
>                   Linus