lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAK7LNAQ-wMTOaCuBab-JOfz4ggefNiEBmiKCUpuFncTqS0P3xQ@mail.gmail.com>
Date: Thu, 7 Nov 2024 16:55:46 +0900
From: Masahiro Yamada <masahiroy@...nel.org>
To: Peter Zijlstra <peterz@...radead.org>
Cc: mcgrof@...nel.org, x86@...nel.org, hpa@...or.com, petr.pavlu@...e.com, 
	samitolvanen@...gle.com, da.gomez@...sung.com, nathan@...nel.org, 
	nicolas@...sle.eu, linux-kernel@...r.kernel.org, 
	linux-modules@...r.kernel.org, linux-kbuild@...r.kernel.org, 
	hch@...radead.org, gregkh@...uxfoundation.org
Subject: Re: [RFC] module: Strict per-modname namespaces

On Thu, Nov 7, 2024 at 4:02 AM Peter Zijlstra <peterz@...radead.org> wrote:
>
> Hi,
>
> I've been wanting $topic for a while, and having just stumbled into the
> whole namespace thing by accident, I figured I'd give it a go, most if
> the hard parts seem to have already been done.
>
> It reserves and disallows imports on any "MODULE_${name}" namespace,
> while it implicitly adds the same namespace to every module.
>
> This allows exports targeted at specific modules and no others -- one
> random example included. I've hated the various kvm exports we've had
> for a while, and strictly limiting them to the kvm module helps
> alleviate some abuse potential.
>
> ---
>  arch/x86/kernel/fpu/core.c |  2 +-
>  kernel/module/main.c       | 28 ++++++++++++++++++++++++++--
>  scripts/mod/modpost.c      | 29 ++++++++++++++++++++---------
>  3 files changed, 47 insertions(+), 12 deletions(-)
>
> diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c
> index 1209c7aebb21..23b188a53d9d 100644
> --- a/arch/x86/kernel/fpu/core.c
> +++ b/arch/x86/kernel/fpu/core.c
> @@ -366,7 +366,7 @@ int fpu_swap_kvm_fpstate(struct fpu_guest *guest_fpu, bool enter_guest)
>         fpregs_unlock();
>         return 0;
>  }
> -EXPORT_SYMBOL_GPL(fpu_swap_kvm_fpstate);
> +EXPORT_SYMBOL_NS_GPL(fpu_swap_kvm_fpstate, MODULE_kvm);
>
>  void fpu_copy_guest_fpstate_to_uabi(struct fpu_guest *gfpu, void *buf,
>                                     unsigned int size, u64 xfeatures, u32 pkru)
> diff --git a/kernel/module/main.c b/kernel/module/main.c
> index 49b9bca9de12..b30af879c2cb 100644
> --- a/kernel/module/main.c
> +++ b/kernel/module/main.c
> @@ -1070,6 +1070,13 @@ static int verify_namespace_is_imported(const struct load_info *info,
>
>         namespace = kernel_symbol_namespace(sym);
>         if (namespace && namespace[0]) {
> +               /*
> +                * Implicitly import MODULE_${mod->name} namespace.
> +                */
> +               if (strncmp(namespace, "MODULE_", 7) == 0 &&
> +                   strcmp(namespace+7, mod->name) == 0)
> +                       return 0;
> +
>                 for_each_modinfo_entry(imported_namespace, info, "import_ns") {
>                         if (strcmp(namespace, imported_namespace) == 0)
>                                 return 0;
> @@ -1613,15 +1620,30 @@ static void module_license_taint_check(struct module *mod, const char *license)
>         }
>  }
>
> -static void setup_modinfo(struct module *mod, struct load_info *info)
> +static int setup_modinfo(struct module *mod, struct load_info *info)
>  {
>         struct module_attribute *attr;
> +       char *imported_namespace;
>         int i;
>
>         for (i = 0; (attr = modinfo_attrs[i]); i++) {
>                 if (attr->setup)
>                         attr->setup(mod, get_modinfo(info, attr->attr.name));
>         }
> +
> +       for_each_modinfo_entry(imported_namespace, info, "import_ns") {
> +               /*
> +                * 'MODULE_' prefixed namespaces are implicit, disallow
> +                * explicit imports.
> +                */
> +               if (strstarts(imported_namespace, "MODULE_")) {
> +                       pr_err("%s: module tries to import module namespace: %s\n",
> +                              mod->name, imported_namespace);
> +                       return -EPERM;
> +               }
> +       }
> +
> +       return 0;
>  }
>
>  static void free_modinfo(struct module *mod)
> @@ -2935,7 +2957,9 @@ static int load_module(struct load_info *info, const char __user *uargs,
>                 goto free_unload;
>
>         /* Set up MODINFO_ATTR fields */
> -       setup_modinfo(mod, info);
> +       err = setup_modinfo(mod, info);
> +       if (err)
> +               goto free_modinfo;
>
>         /* Fix up syms, so that st_value is a pointer to location. */
>         err = simplify_symbols(mod, info);
> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> index 107393a8c48a..d1de3044ee03 100644
> --- a/scripts/mod/modpost.c
> +++ b/scripts/mod/modpost.c
> @@ -1553,8 +1553,19 @@ static void mod_set_crcs(struct module *mod)
>         free(buf);
>  }
>
> +static const char *mod_basename(const char *modname)
> +{
> +       const char *basename = strrchr(modname, '/');
> +       if (basename)
> +               basename++;
> +       else
> +               basename = modname;
> +       return basename;
> +}
> +
>  static void read_symbols(const char *modname)
>  {
> +       char module_namespace[MODULE_NAME_LEN + 8];
>         const char *symname;
>         char *version;
>         char *license;
> @@ -1586,12 +1597,16 @@ static void read_symbols(const char *modname)
>                         license = get_next_modinfo(&info, "license", license);
>                 }
>
> -               namespace = get_modinfo(&info, "import_ns");
> -               while (namespace) {
> +               for (namespace = get_modinfo(&info, "import_ns"); namespace;
> +                    namespace = get_next_modinfo(&info, "import_ns", namespace)) {

The conversion from while() to for() is an unrelated change.
Split it to a separate patch if you want to change it.


> +                       if (strstarts(namespace, "MODULE_"))
> +                               error("importing implicit module namespace: %s\n", namespace);
> +
>                         add_namespace(&mod->imported_namespaces, namespace);
> -                       namespace = get_next_modinfo(&info, "import_ns",
> -                                                    namespace);
>                 }
> +               snprintf(module_namespace, sizeof(module_namespace), "MODULE_%s",
> +                        mod_basename(mod->name));
> +               add_namespace(&mod->imported_namespaces, module_namespace);
>
>                 if (extra_warn && !get_modinfo(&info, "description"))
>                         warn("missing MODULE_DESCRIPTION() in %s\n", modname);
> @@ -1700,11 +1715,7 @@ static void check_exports(struct module *mod)
>                 s->crc_valid = exp->crc_valid;
>                 s->crc = exp->crc;
>
> -               basename = strrchr(mod->name, '/');
> -               if (basename)
> -                       basename++;
> -               else
> -                       basename = mod->name;
> +               basename = mod_basename(mod->name);

This is an unrelated change.

So, it should be split into a separate prerequisite patch,
something like, "modpost: introduce mod_basename() helper"



>                 if (!contains_namespace(&mod->imported_namespaces, exp->namespace)) {
>                         modpost_log(!allow_missing_ns_imports,



-- 
Best Regards
Masahiro Yamada

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ