Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.31 as permitted sender) client-ip=23.128.96.31;
MIME-Version: 1.0
References: <20231006051801.423973-1-sumit.garg@linaro.org>
In-Reply-To: <20231006051801.423973-1-sumit.garg@linaro.org>
From:   Linus Torvalds <torvalds@linux-foundation.org>
Date:   Tue, 10 Oct 2023 11:28:49 -0700
Message-ID: <CAHk-=whOPoLaWM8S8GgoOPT7a2+nMH5h3TLKtn=R_3w4R1_Uvg@mail.gmail.com>
Subject: Re: [PATCH v2] KEYS: trusted: Remove redundant static calls usage
To:     Sumit Garg <sumit.garg@linaro.org>,
        David Howells <dhowells@redhat.com>
Cc:     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 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.

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.

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. But as mentioned, this is a separate issue
from the whole "you currently can't do static calls from __exit
functions" issue.

                  Linus