Hi!

Implement a means for exports to be available only to an explicit list of named
modules. By explicitly limiting the usage of certain exports, the abuse
potential/risk is greatly reduced.

The first three 'patches' clean up the existing export namespace code along the
same lines of 33def8498fdd ("treewide: Convert macro and uses of __section(foo)
to __section("foo")") and for the same reason, it is not desired for the
namespace argument to be a macro expansion itself.

In fact, the second patch is really only a script, because sending the output
to the list is a giant waste of bandwidth. Whoever eventually commits this to a
git tree should squash these first three patches.

The remainder of the patches introduce the special "MODULE_<modname-list>"
namespace, which shall be forbidden from being explicitly imported. A module
that matches the simple modname-list will get an implicit import.

Lightly tested with something like:

git grep -l EXPORT_SYMBOL arch/x86/kvm/ | while read file;
do
  sed -i -e 's/EXPORT_SYMBOL_GPL(\(.[^)]*\))/EXPORT_SYMBOL_GPL_FOR(\1, "kvm,kvm-intel,kvm-amd")/g' $file;
done

Also available at:

  git://git.kernel.org/pub/scm/linux/kernel/git/peterz/queue.git module/namespace

On Mon, Nov 11, 2024, Peter Zijlstra wrote:
> Hi!
> 
> Implement a means for exports to be available only to an explicit list of named
> modules. By explicitly limiting the usage of certain exports, the abuse
> potential/risk is greatly reduced.
> 
> The first three 'patches' clean up the existing export namespace code along the
> same lines of 33def8498fdd ("treewide: Convert macro and uses of __section(foo)
> to __section("foo")") and for the same reason, it is not desired for the
> namespace argument to be a macro expansion itself.
> 
> In fact, the second patch is really only a script, because sending the output
> to the list is a giant waste of bandwidth. Whoever eventually commits this to a
> git tree should squash these first three patches.
> 
> The remainder of the patches introduce the special "MODULE_<modname-list>"
> namespace, which shall be forbidden from being explicitly imported. A module
> that matches the simple modname-list will get an implicit import.
> 
> Lightly tested with something like:
> 
> git grep -l EXPORT_SYMBOL arch/x86/kvm/ | while read file;
> do
>   sed -i -e 's/EXPORT_SYMBOL_GPL(\(.[^)]*\))/EXPORT_SYMBOL_GPL_FOR(\1, "kvm,kvm-intel,kvm-amd")/g' $file;
> done

Heh, darn modules.  This will compile just fine, but if the module contains a
dash, loading the module will fail because scripts/Makefile.lib replaces the dash
with an underscore the build name.  E.g. "kvm-intel" at compile time generates
kvm-intel.ko, but the actual name of the module as seen by the kernel is kvm_intel.

--
# These flags are needed for modversions and compiling, so we define them here
# $(modname_flags) defines KBUILD_MODNAME as the name of the module it will
# end up in (or would, if it gets compiled in)
name-fix-token = $(subst $(comma),_,$(subst -,_,$1))  <====================
name-fix = $(call stringify,$(call name-fix-token,$1))
basename_flags = -DKBUILD_BASENAME=$(call name-fix,$(basetarget))
modname_flags  = -DKBUILD_MODNAME=$(call name-fix,$(modname)) \
		 -D__KBUILD_MODNAME=kmod_$(call name-fix-token,$(modname))
modfile_flags  = -DKBUILD_MODFILE=$(call stringify,$(modfile))
--

On Mon, Nov 11, 2024 at 04:48:58PM -0800, Sean Christopherson wrote:
> On Mon, Nov 11, 2024, Peter Zijlstra wrote:
> > Hi!
> > 
> > Implement a means for exports to be available only to an explicit list of named
> > modules. By explicitly limiting the usage of certain exports, the abuse
> > potential/risk is greatly reduced.
> > 
> > The first three 'patches' clean up the existing export namespace code along the
> > same lines of 33def8498fdd ("treewide: Convert macro and uses of __section(foo)
> > to __section("foo")") and for the same reason, it is not desired for the
> > namespace argument to be a macro expansion itself.
> > 
> > In fact, the second patch is really only a script, because sending the output
> > to the list is a giant waste of bandwidth. Whoever eventually commits this to a
> > git tree should squash these first three patches.
> > 
> > The remainder of the patches introduce the special "MODULE_<modname-list>"
> > namespace, which shall be forbidden from being explicitly imported. A module
> > that matches the simple modname-list will get an implicit import.
> > 
> > Lightly tested with something like:
> > 
> > git grep -l EXPORT_SYMBOL arch/x86/kvm/ | while read file;
> > do
> >   sed -i -e 's/EXPORT_SYMBOL_GPL(\(.[^)]*\))/EXPORT_SYMBOL_GPL_FOR(\1, "kvm,kvm-intel,kvm-amd")/g' $file;
> > done
> 
> Heh, darn modules.  This will compile just fine, but if the module contains a
> dash, loading the module will fail because scripts/Makefile.lib replaces the dash
> with an underscore the build name.  E.g. "kvm-intel" at compile time generates
> kvm-intel.ko, but the actual name of the module as seen by the kernel is kvm_intel.

I was wondering about that...  WTH is kvm doing that? I mean, I suppose
you can do: "kvm-intel,kvm_intel" but that's somewhat tedious.

On Tue, Nov 12, 2024, Peter Zijlstra wrote:
> On Mon, Nov 11, 2024 at 04:48:58PM -0800, Sean Christopherson wrote:
> > On Mon, Nov 11, 2024, Peter Zijlstra wrote:
> > > Hi!
> > > 
> > > Implement a means for exports to be available only to an explicit list of named
> > > modules. By explicitly limiting the usage of certain exports, the abuse
> > > potential/risk is greatly reduced.
> > > 
> > > The first three 'patches' clean up the existing export namespace code along the
> > > same lines of 33def8498fdd ("treewide: Convert macro and uses of __section(foo)
> > > to __section("foo")") and for the same reason, it is not desired for the
> > > namespace argument to be a macro expansion itself.
> > > 
> > > In fact, the second patch is really only a script, because sending the output
> > > to the list is a giant waste of bandwidth. Whoever eventually commits this to a
> > > git tree should squash these first three patches.
> > > 
> > > The remainder of the patches introduce the special "MODULE_<modname-list>"
> > > namespace, which shall be forbidden from being explicitly imported. A module
> > > that matches the simple modname-list will get an implicit import.
> > > 
> > > Lightly tested with something like:
> > > 
> > > git grep -l EXPORT_SYMBOL arch/x86/kvm/ | while read file;
> > > do
> > >   sed -i -e 's/EXPORT_SYMBOL_GPL(\(.[^)]*\))/EXPORT_SYMBOL_GPL_FOR(\1, "kvm,kvm-intel,kvm-amd")/g' $file;
> > > done
> > 
> > Heh, darn modules.  This will compile just fine, but if the module contains a
> > dash, loading the module will fail because scripts/Makefile.lib replaces the dash
> > with an underscore the build name.  E.g. "kvm-intel" at compile time generates
> > kvm-intel.ko, but the actual name of the module as seen by the kernel is kvm_intel.
> 
> I was wondering about that...  WTH is kvm doing that?

No idea.  The naming has been that way since KVM's inception in commit 6aa8b732ca01
("[PATCH] kvm: userspace interface").  My guess is that either no one noticed, or
those who noticed didn't care.

FWIW, IMO the kernel build system is the one that's being weird.  AFAICT, the
'-' => '_' conversion was added so that spinlocks could be placed into unique
subsections.  Amusingly, it doesn't appear that there are any remaining users of
LOCK_SECTION_NAME.

  commit b5635319d32438ed516568f53013a460ba16e6ee
  Author:     Dave Jones <davej@suse.de>
  AuthorDate: Fri Feb 8 01:43:23 2002 -0800
  Commit:     Linus Torvalds <torvalds@penguin.transmeta.com>
  CommitDate: Fri Feb 8 01:43:23 2002 -0800

    [PATCH] text.lock -> subsection changes.
    
    Make spinlocks etc use subsections of their parent sections instead of
    an ELF section of their own - needed for newer binutils when the parent
    sector is removed.

#define LOCK_SECTION_NAME ".text..lock."KBUILD_BASENAME

#define LOCK_SECTION_START(extra)               \
        ".subsection 1\n\t"                     \
        extra                                   \
        ".ifndef " LOCK_SECTION_NAME "\n\t"     \
        LOCK_SECTION_NAME ":\n\t"               \
        ".endif\n"

#define LOCK_SECTION_END                        \
        ".previous\n\t"

#define __lockfunc __section(".spinlock.text")


> I mean, I suppose you can do: "kvm-intel,kvm_intel" but that's somewhat
> tedious.

This likely needs to be addressed in whatever chunk of code is enforcing the
namespaces.  The s/-/_ behavior (and vice versa!) is *very* baked into the kernel
at this point, e.g. parameqn() will happily parse dashes or underscores for every
kernel parameter.  As horrific as it is, I think the module namespace needs to do
the same, i.e. treat dashes and underscores as one and the same.


More historical amusement:

commit 8863179c65618844379ef90d4a708293042465c8
Author:     Andrew Morton <akpm@digeo.com>
AuthorDate: Sun Feb 2 06:08:27 2003 -0800
Commit:     Linus Torvalds <torvalds@home.transmeta.com>
CommitDate: Sun Feb 2 06:08:27 2003 -0800

    [PATCH] kernel param and KBUILD_MODNAME name-munging mess
    
    Patch from: Rusty Russell <rusty@rustcorp.com.au>
    
    Mikael Pettersson points out that "-s" gets mangled to "_s" on the
    kernel command line, even though it turns out not to be a
    parameter.

commit 326e7842d30d5cfc1089b85a7aa63e5c9f3c0a74
Author:     Rusty Russell <rusty@rustcorp.com.au>
AuthorDate: Sat Dec 14 20:13:11 2002 -0800
Commit:     Linus Torvalds <torvalds@home.transmeta.com>
CommitDate: Sat Dec 14 20:13:11 2002 -0800

    [PATCH] Module Parameter Core Patch
    
    This patch is a rewrite of the insmod and boot parameter handling,
    to unify them.
    
    The new format is fairly simple: built on top of __module_param_call there
    are several helpers, eg "module_param(foo, int, 000)".  The final argument
    is the permissions bits, for exposing parameters in sysfs (if
    non-zero) at a later stage.

On Tue, Nov 12, 2024 at 09:56:20AM -0800, Sean Christopherson wrote:

> This likely needs to be addressed in whatever chunk of code is enforcing the
> namespaces.  The s/-/_ behavior (and vice versa!) is *very* baked into the kernel
> at this point, e.g. parameqn() will happily parse dashes or underscores for every
> kernel parameter.  As horrific as it is, I think the module namespace needs to do
> the same, i.e. treat dashes and underscores as one and the same.

Right, I'll add a s/-/_/g on both ends of the strcmp or somesuch.

On Tue, Nov 12, 2024 at 08:52:26PM +0100, Peter Zijlstra wrote:
> On Tue, Nov 12, 2024 at 09:56:20AM -0800, Sean Christopherson wrote:
> 
> > This likely needs to be addressed in whatever chunk of code is enforcing the
> > namespaces.  The s/-/_ behavior (and vice versa!) is *very* baked into the kernel
> > at this point, e.g. parameqn() will happily parse dashes or underscores for every
> > kernel parameter.  As horrific as it is, I think the module namespace needs to do
> > the same, i.e. treat dashes and underscores as one and the same.
> 
> Right, I'll add a s/-/_/g on both ends of the strcmp or somesuch.

https://git.kernel.org/pub/scm/linux/kernel/git/peterz/queue.git/commit/?h=module/namespace&id=f2aabf8436348a47037570af139ec2c1de8c5337

My test box seems able to load kvm_intel. I'll let it all sit in that
tree for a few days such that 0day might get a chance to chew on it
before posting.

Thanks!

git grep -l -e MODULE_IMPORT_NS -e EXPORT_SYMBOL_NS | while read file;
do
  sed -i -e 's/MODULE_IMPORT_NS(\([^)]*\))/MODULE_IMPORT_NS("\1")/g' \
         -e 's/\(EXPORT_SYMBOL_NS[^(]*\)(\([^,]*\), \([^)]*\))/\1(\2, "\3")/g' $file;
done

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---