Re: [PATCH] module : fix signature checker pointer arithmetic and bounds check

[Christophe Leroy <christophe.leroy@csgroup.eu>]

Le 05/09/2025 à 17:45, Fidal Palamparambil a écrit :
> From: Fidal palamparambil <rootuserhere@gmail.com>
> 
> This patch fixes :
>   - invalid module_param type (bool_enable_only → bool)

Can you explain what the problem is ? Why do you say bool_enable_only is 
invalid ? Was generalised by commit d19f05d8a8fa ("kernel/params.c: 
generalize bool_enable_only")

>   - unsafe pointer arithmetic on void *

Why is it unsafe in Linux Kernel ? See https://lkml.org/lkml/2022/2/24/978

>   - missing bounds check for sig_len, preventing underflow/OOB

This is checked by mod_check_sig(), why check it again ?

>   - export set_module_sig_enforced for consistency

Consistency with what ? Can you tell which module needs it ?

> 
> Signed-off-by : Fidal Palamparambil <rootuserhere@gmail.com>
> Signed-off-by: Fidal palamparambil <rootuserhere@gmail.com>

Why a double sob ?

> ---
>   kernel/module/signing.c    |  48 ++++++++------
>   kernel/module/signing.orig | 125 +++++++++++++++++++++++++++++++++++++

Why adding this .orig file into the kernel at all ?

>   2 files changed, 155 insertions(+), 18 deletions(-)
>   create mode 100644 kernel/module/signing.orig
> 
> diff --git a/kernel/module/signing.c b/kernel/module/signing.c
> index a2ff4242e623..8dda6cd2fd73 100644
> --- a/kernel/module/signing.c
> +++ b/kernel/module/signing.c
> @@ -1,5 +1,6 @@
>   // SPDX-License-Identifier: GPL-2.0-or-later
> -/* Module signature checker
> +/*
> + * Module signature checker

Don't mix cosmetic changes and real changes, you are making 
bisectability more difficult.

>    *
>    * Copyright (C) 2012 Red Hat, Inc. All Rights Reserved.
>    * Written by David Howells (dhowells@redhat.com)
> @@ -20,11 +21,11 @@
>   #define MODULE_PARAM_PREFIX "module."
> 
>   static bool sig_enforce = IS_ENABLED(CONFIG_MODULE_SIG_FORCE);
> -module_param(sig_enforce, bool_enable_only, 0644);
> +module_param(sig_enforce, bool, 0644);
> 
>   /*
> - * Export sig_enforce kernel cmdline parameter to allow other subsystems rely
> - * on that instead of directly to CONFIG_MODULE_SIG_FORCE config.
> + * Export sig_enforce kernel cmdline parameter to allow other subsystems to
> + * rely on that instead of directly on CONFIG_MODULE_SIG_FORCE config.
>    */
>   bool is_module_sig_enforced(void)
>   {
> @@ -36,6 +37,7 @@ void set_module_sig_enforced(void)
>   {
>          sig_enforce = true;
>   }
> +EXPORT_SYMBOL(set_module_sig_enforced);
> 
>   /*
>    * Verify the signature on a module.
> @@ -45,44 +47,55 @@ int mod_verify_sig(const void *mod, struct load_info *info)
>          struct module_signature ms;
>          size_t sig_len, modlen = info->len;
>          int ret;
> +       const unsigned char *data = mod;

Pointless change.

> 
>          pr_devel("==>%s(,%zu)\n", __func__, modlen);
> 
>          if (modlen <= sizeof(ms))
>                  return -EBADMSG;
> 
> -       memcpy(&ms, mod + (modlen - sizeof(ms)), sizeof(ms));
> +       memcpy(&ms, data + (modlen - sizeof(ms)), sizeof(ms));

Pointless change

> 
>          ret = mod_check_sig(&ms, modlen, "module");
>          if (ret)
>                  return ret;
> 
>          sig_len = be32_to_cpu(ms.sig_len);
> +
> +       /* Ensure sig_len is valid to prevent underflow/oob */
> +       if (sig_len > modlen - sizeof(ms))
> +               return -EBADMSG;

Already verified by mod_check_sig()

> +
>          modlen -= sig_len + sizeof(ms);
>          info->len = modlen;
> 
> -       return verify_pkcs7_signature(mod, modlen, mod + modlen, sig_len,
> +       return verify_pkcs7_signature(data, modlen, data + modlen, sig_len,

pointless change

>                                        VERIFY_USE_SECONDARY_KEYRING,
>                                        VERIFYING_MODULE_SIGNATURE,
>                                        NULL, NULL);
>   }
> 
> +/*
> + * Check signature validity of a module during load.
> + */
>   int module_sig_check(struct load_info *info, int flags)
>   {
>          int err = -ENODATA;
>          const unsigned long markerlen = sizeof(MODULE_SIG_STRING) - 1;
>          const char *reason;
> -       const void *mod = info->hdr;
> +       const unsigned char *mod = info->hdr;

info->hdr is not void*, how can this work without a cast ?

>          bool mangled_module = flags & (MODULE_INIT_IGNORE_MODVERSIONS |
>                                         MODULE_INIT_IGNORE_VERMAGIC);
> +

Unrelated cosmetic change

>          /*
> -        * Do not allow mangled modules as a module with version information
> -        * removed is no longer the module that was signed.
> +        * Do not allow mangled modules: a module with version info removed
> +        * is no longer the module that was signed.
>           */
>          if (!mangled_module &&
>              info->len > markerlen &&
> -           memcmp(mod + info->len - markerlen, MODULE_SIG_STRING, markerlen) == 0) {
> -               /* We truncate the module to discard the signature */
> +           memcmp(mod + info->len - markerlen,
> +                  MODULE_SIG_STRING, markerlen) == 0) {
> +               /* Truncate the module to discard the signature marker */

Cosmetic and pointless change.

>                  info->len -= markerlen;
>                  err = mod_verify_sig(mod, info);
>                  if (!err) {
> @@ -92,9 +105,8 @@ int module_sig_check(struct load_info *info, int flags)
>          }
> 
>          /*
> -        * We don't permit modules to be loaded into the trusted kernels
> -        * without a valid signature on them, but if we're not enforcing,
> -        * certain errors are non-fatal.
> +        * Enforced mode: only allow modules with a valid signature.
> +        * Non-enforced mode: certain errors are downgraded to warnings.
>           */
>          switch (err) {
>          case -ENODATA:
> @@ -106,12 +118,12 @@ int module_sig_check(struct load_info *info, int flags)
>          case -ENOKEY:
>                  reason = "module with unavailable key";
>                  break;
> -

Cosmetic

>          default:
>                  /*
> -                * All other errors are fatal, including lack of memory,
> -                * unparseable signatures, and signature check failures --
> -                * even if signatures aren't required.
> +                * All other errors are fatal, including:
> +                * - OOM
> +                * - unparseable signatures
> +                * - invalid signature failures
>                   */
>                  return err;
>          }
> diff --git a/kernel/module/signing.orig b/kernel/module/signing.orig
> new file mode 100644
> index 000000000000..a2ff4242e623
> --- /dev/null
> +++ b/kernel/module/signing.orig
> @@ -0,0 +1,125 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/* Module signature checker
> + *
> + * Copyright (C) 2012 Red Hat, Inc. All Rights Reserved.
> + * Written by David Howells (dhowells@redhat.com)
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/errno.h>
> +#include <linux/module.h>
> +#include <linux/module_signature.h>
> +#include <linux/string.h>
> +#include <linux/verification.h>
> +#include <linux/security.h>
> +#include <crypto/public_key.h>
> +#include <uapi/linux/module.h>
> +#include "internal.h"
> +
> +#undef MODULE_PARAM_PREFIX
> +#define MODULE_PARAM_PREFIX "module."
> +
> +static bool sig_enforce = IS_ENABLED(CONFIG_MODULE_SIG_FORCE);
> +module_param(sig_enforce, bool_enable_only, 0644);
> +
> +/*
> + * Export sig_enforce kernel cmdline parameter to allow other subsystems rely
> + * on that instead of directly to CONFIG_MODULE_SIG_FORCE config.
> + */
> +bool is_module_sig_enforced(void)
> +{
> +       return sig_enforce;
> +}
> +EXPORT_SYMBOL(is_module_sig_enforced);
> +
> +void set_module_sig_enforced(void)
> +{
> +       sig_enforce = true;
> +}
> +
> +/*
> + * Verify the signature on a module.
> + */
> +int mod_verify_sig(const void *mod, struct load_info *info)
> +{
> +       struct module_signature ms;
> +       size_t sig_len, modlen = info->len;
> +       int ret;
> +
> +       pr_devel("==>%s(,%zu)\n", __func__, modlen);
> +
> +       if (modlen <= sizeof(ms))
> +               return -EBADMSG;
> +
> +       memcpy(&ms, mod + (modlen - sizeof(ms)), sizeof(ms));
> +
> +       ret = mod_check_sig(&ms, modlen, "module");
> +       if (ret)
> +               return ret;
> +
> +       sig_len = be32_to_cpu(ms.sig_len);
> +       modlen -= sig_len + sizeof(ms);
> +       info->len = modlen;
> +
> +       return verify_pkcs7_signature(mod, modlen, mod + modlen, sig_len,
> +                                     VERIFY_USE_SECONDARY_KEYRING,
> +                                     VERIFYING_MODULE_SIGNATURE,
> +                                     NULL, NULL);
> +}
> +
> +int module_sig_check(struct load_info *info, int flags)
> +{
> +       int err = -ENODATA;
> +       const unsigned long markerlen = sizeof(MODULE_SIG_STRING) - 1;
> +       const char *reason;
> +       const void *mod = info->hdr;
> +       bool mangled_module = flags & (MODULE_INIT_IGNORE_MODVERSIONS |
> +                                      MODULE_INIT_IGNORE_VERMAGIC);
> +       /*
> +        * Do not allow mangled modules as a module with version information
> +        * removed is no longer the module that was signed.
> +        */
> +       if (!mangled_module &&
> +           info->len > markerlen &&
> +           memcmp(mod + info->len - markerlen, MODULE_SIG_STRING, markerlen) == 0) {
> +               /* We truncate the module to discard the signature */
> +               info->len -= markerlen;
> +               err = mod_verify_sig(mod, info);
> +               if (!err) {
> +                       info->sig_ok = true;
> +                       return 0;
> +               }
> +       }
> +
> +       /*
> +        * We don't permit modules to be loaded into the trusted kernels
> +        * without a valid signature on them, but if we're not enforcing,
> +        * certain errors are non-fatal.
> +        */
> +       switch (err) {
> +       case -ENODATA:
> +               reason = "unsigned module";
> +               break;
> +       case -ENOPKG:
> +               reason = "module with unsupported crypto";
> +               break;
> +       case -ENOKEY:
> +               reason = "module with unavailable key";
> +               break;
> +
> +       default:
> +               /*
> +                * All other errors are fatal, including lack of memory,
> +                * unparseable signatures, and signature check failures --
> +                * even if signatures aren't required.
> +                */
> +               return err;
> +       }
> +
> +       if (is_module_sig_enforced()) {
> +               pr_notice("Loading of %s is rejected\n", reason);
> +               return -EKEYREJECTED;
> +       }
> +
> +       return security_locked_down(LOCKDOWN_MODULE_SIGNATURE);
> +}
> --
> 2.50.1.windows.1
> 
>