On Wed, Apr 19, 2017 at 4:43 PM, Kees Cook [off-list ref] wrote:

On Wed, Apr 19, 2017 at 4:15 PM, Andy Lutomirski [off-list ref] wrote:

quoted

On Wed, Apr 19, 2017 at 3:20 PM, Djalal Harouni [off-list ref] wrote:

quoted

+/* Sets task's modules_autoload */
+static inline int task_set_modules_autoload(struct task_struct *task,
+                                           unsigned long value)
+{
+       if (value > MODULES_AUTOLOAD_DISABLED)
+               return -EINVAL;
+       else if (task->modules_autoload > value)
+               return -EPERM;
+       else if (task->modules_autoload < value)
+               task->modules_autoload = value;
+
+       return 0;
+}

This needs to be more locked down.  Otherwise someone could set this
and then run a setuid program.  Admittedly, it would be quite odd if
this particular thing causes a problem, but the issue exists
nonetheless.

Eeeh, I don't agree this needs to be changed. APIs provided by modules
are different than the existing privilege-manipulation syscalls this
concern stems from. Applications are already forced to deal with
things being missing like this in the face of it simply not being
built into the kernel.

Having to hide this behind nnp seems like it'd reduce its utility...

I think that adding an inherited boolean to task_struct that can be
set by unprivileged tasks and passed to privileged tasks is a terrible
precedent.  Ideally someone would try to find all the existing things
like this and kill them off.

I agree that I don't see how one would exploit this particular
feature, but I still think I dislike the approach.  This is a slippery
slope to adding a boolean for perf_event_open(), unshare(), etc, and
we should solve these for real rather than half-arsing them IMO.