Re: [PATCH] kmod: don't load module unless req process has CAP_SYS_MODULE

[Eric Dumazet]

On Sun, May 14, 2017 at 7:42 PM, Mahesh Bandewar (àààà ààààààà)
<maheshb@xxxxxxxxxx> wrote:
> On Sun, May 14, 2017 at 3:45 AM, Greg Kroah-Hartman
> <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
>> On Fri, May 12, 2017 at 04:22:59PM -0700, Mahesh Bandewar wrote:
>>> From: Mahesh Bandewar <maheshb@xxxxxxxxxx>
>>>
> [...]
>>>   Now try to create a bridge inside this newly created net-ns which would
>>>   mean bridge module need to be loaded.
>>>   # ip link add br0 type bridge
>>>   # echo $?
>>>   0
>>>   # lsmod | grep bridge
>>>   bridge                110592  0
>>>   stp                    16384  1 bridge
>>>   llc                    16384  2 bridge,stp
>>>   #
>>>
>>>   After this patch -
>>>   # ip link add br0 type bridge
>>>   RTNETLINK answers: Operation not supported
>>>   # echo $?
>>>   2
>>>   # lsmod | grep bridge
>>>   #
>>
>> Well, it only loads this because the kernel asked for it to be loaded,
>> right?
>>
> Yes, kernel asked for it because of a user action.
>
>>>
>>> Signed-off-by: Mahesh Bandewar <maheshb@xxxxxxxxxx>
>>> ---
>>>  kernel/kmod.c | 3 +++
>>>  1 file changed, 3 insertions(+)
>>>
>>> diff --git a/kernel/kmod.c b/kernel/kmod.c
>>> index 563f97e2be36..ac30157169b7 100644
>>> --- a/kernel/kmod.c
>>> +++ b/kernel/kmod.c
>>> @@ -133,6 +133,9 @@ int __request_module(bool wait, const char *fmt, ...)
>>>  #define MAX_KMOD_CONCURRENT 50       /* Completely arbitrary value - KAO */
>>>       static int kmod_loop_msg;
>>>
>>> +     if (!capable(CAP_SYS_MODULE))
>>> +             return -EPERM;
>>
>> At first glance this looks right, but I'm worried what this will break
>> that currently relies on this.  There might be lots of systems that are
>> used to this being the method that the needed module is requested.  What
>> about when userspace asks for a random char device and that module is
>> then loaded?  Does this patch break that functionality?
>>
> Any module when loaded gets loaded system-wide as we can't allow
> module loading per-ns. To validate the behavior I was comparing it
> with insmod/modprobe, if that doesn't allow because of lack of this
> capability in default-ns, then this *indirect* method of loading
> module should not allow the same action and the behavior should be
> consistent. So with that logic if userspace asks for a random
> char-device if insmod/modprobe cannot load it, then this method should
> not load it either for the consistency, right?


This patch will break applications that expected modules being auto loaded.

Try to use SCTP protocol if module is not loaded.

Current kernels :

SCTP is (auto) loaded, application can use SCTP just fine.

After your patch : socket() will fail, unless application run by a
privileged user.

Some people will qualify this as a regression.