On Mon, Apr 10, 2017 at 5:42 PM, Casey Schaufler [off-list ref] wrote:

On 4/9/2017 3:42 AM, Djalal Harouni wrote:

[...]

quoted

+
+static inline struct modautoload_task *init_modautoload_task(struct task_struct *tsk,
+                                                          unsigned long flags)
+{
+     struct modautoload_task *modtask;
+
+     modtask = task_security(tsk, modautorestrict_task_security_index);
+
+     modtask->flags = (u8)flags;

I don't think you want to do this cast.

Will fix it. Thanks!


[...]

quoted

+
+#ifdef CONFIG_SYSCTL
+static int modautoload_dointvec_minmax(struct ctl_table *table, int write,
+                                    void __user *buffer, size_t *lenp,
+                                    loff_t *ppos)
+{
+     struct ctl_table table_copy;
+
+     if (write && !capable(CAP_SYS_MODULE))
+             return -EPERM;
+
+     table_copy = *table;
+     if (*(int *)table_copy.data == *(int *)table_copy.extra2)

While it's probably doing what you want, I find this
sort of casting disturbing.

Ok will try to improve it.

quoted

+             table_copy.extra1 = table_copy.extra2;
+
+     return proc_dointvec_minmax(&table_copy, write, buffer, lenp, ppos);
+}
+
+struct ctl_path modautoload_sysctl_path[] = {
+     { .procname = "kernel", },
+     { .procname = "modautorestrict", },
+     { }
+};
+
+static struct ctl_table modautoload_sysctl_table[] = {
+     {
+             .procname       = "autoload",
+             .data           = &autoload_restrict,
+             .maxlen         = sizeof(int),
+             .mode           = 0644,
+             .proc_handler   = modautoload_dointvec_minmax,
+             .extra1         = &zero,
+             .extra2         = &max_autoload_restrict,
+     },
+     { }
+};
+
+static void __init modautoload_init_sysctl(void)
+{
+     if (!register_sysctl_paths(modautoload_sysctl_path, modautoload_sysctl_table))
+             panic("modautorestrict: sysctl registration failed.\n");
+}
+#else
+static inline void modautoload_init_sysctl(void) { }
+#endif /* CONFIG_SYSCTL */
+
+void __init modautorestrict_init(void)
+{
+     modautorestrict_task_security_index =
+             security_reserve_task_blob_index(sizeof(struct modautoload_task));
+     security_add_hooks(modautoload_hooks,
+                        ARRAY_SIZE(modautoload_hooks), "modautorestrict");
+
+     modautoload_init_sysctl();
+     pr_info("ModAutoRestrict LSM:  Initialized\n");
+}

diff --git a/security/security.c b/security/security.c
index 4dc6bca..d8852fe 100644
--- a/security/security.c
+++ b/security/security.c

@@ -70,6 +70,7 @@ int __init security_init(void)
      capability_add_hooks();
      yama_add_hooks();
      loadpin_add_hooks();
+     modautorestrict_init();

This should be modautorestrict_add_hooks() if this were
a "minor" module, but as it's using a blob it is a "major"
module. Either way, this is not right.

Do you mean that if I'm using a blob, it should go with the rest LSMs
in do_security_initcalls() ?

quoted

      /*
       * Load all the remaining security modules.

Thanks for the comments!


-- 
tixxdz