* frm chrisat_private "10/16/2001 07:34:42 PM -0700" | sed '1,$s/^/* /' * * *> ===== kernel/module.c 1.16 vs edited ===== *> @@ -357,8 +357,6 @@ *> unsigned long mod_user_size; *> struct module_ref *dep; *> *> - if (!capable(CAP_SYS_MODULE)) *> - return -EPERM; *> lock_kernel(); *> if ((namelen = get_mod_name(name_user, &name)) < 0) { *> error = namelen; * * am i missing something here? did you just delete the CAP_SYS_MODULE * check? this surely isn't right! Opps, you're right. The intention was to move the capable call nearer the hook, but that is so far into the function that it might be better to move the hook further forward (to limit grottiness), simmilar to how create_module is handled... @@ -353,12 +353,10 @@ { struct module mod_tmp, *mod; char *name, *n_name, *name_tmp = NULL; - long namelen, n_namelen, i, error; + long namelen, n_namelen, i, error=0; unsigned long mod_user_size; struct module_ref *dep; - if (!capable(CAP_SYS_MODULE)) - return -EPERM; lock_kernel(); if ((namelen = get_mod_name(name_user, &name)) < 0) { error = namelen; @@ -369,6 +367,14 @@ goto err1; } + /* check that we have permission to do this */ + if (!capable(CAP_SYS_MODULE)) + error = -EPERM; + + error = security_ops->module_ops->init_module(error,mod); + if (error) + goto err1; + /* Check module header size. We allow a bit of slop over the size we are familiar with to cope with a version of insmod for a newer kernel. But don't over do it. */ @@ -504,10 +510,6 @@ goto err3; } - /* check that we have permission to do this */ - error = security_ops->module_ops->init_module(mod); - if (error) - goto err3; error = -EINVAL; if (module_arch_init(mod)) Does moving the hook before the code sanity checks the module impact anyone ? * *> @@ -632,7 +627,11 @@ *> spin_lock(&unload_lock); *> if (!__MOD_IN_USE(mod)) { *> /* check that we have permission to do this */ *> - error = security_ops->module_ops->delete_module(mod); *> + error = 0; *> + if (!capable(CAP_SYS_MODULE)) *> + error = -EPERM; *> + *> + error = security_ops->module_ops->delete_module(error,mod); *> if (error) { *> spin_unlock(&unload_lock); *> goto out; *> @@ -645,6 +644,15 @@ *> spin_unlock(&unload_lock); *> } *> goto out; *> + } else { *> + /* check that we have permission to do this */ *> + error = 0; *> + if (!capable(CAP_SYS_MODULE)) *> + error = -EPERM; *> + *> + error = security_ops->module_ops->delete_module(error, NULL); *> + if (error) *> + goto out; * * so now, delete_module is called twice in the automatic reaping case. * once with a NULL (struct mod*). i suppose this is better than the * alternative of checking CAP_SYS_MODULE for every module during reaping, * but it's a little kludgy feeling. and, a change like this would need * some corresponding docs...to let module developer know that (struct mod*) * may be NULL. * Do you think it would be better to do it this way without passing NULL to the hook (and eliminating the extra hook call): asmlinkage long sys_delete_module(const char *name_user) { struct module *mod, *next; char *name; long error; int something_changed; lock_kernel(); if (name_user) { if ((error = get_mod_name(name_user, &name)) < 0) goto out; error = -ENOENT; if ((mod = find_module(name)) == NULL) { put_mod_name(name); goto out; } put_mod_name(name); error = -EBUSY; if (mod->refs != NULL) goto out; spin_lock(&unload_lock); if (!__MOD_IN_USE(mod)) { /* check that we have permission to do this */ error = 0; if (!capable(CAP_SYS_MODULE)) error = -EPERM; error = security_ops->module_ops->delete_module(error,mod); if (error) { spin_unlock(&unload_lock); goto out; } mod->flags |= MOD_DELETED; spin_unlock(&unload_lock); free_module(mod, 0); error = 0; } else { spin_unlock(&unload_lock); } goto out; } /* check that we have permission to do this */ error = -EPERM; if (!capable(CAP_SYS_MODULE)) goto out; /* Do automatic reaping */ restart: something_changed = 0; for (mod = module_list; mod != &kernel_module; mod = next) { next = mod->next; spin_lock(&unload_lock); if (mod->refs == NULL && (mod->flags & MOD_AUTOCLEAN) && (mod->flags & MOD_RUNNING) && !(mod->flags & MOD_DELETED) && (mod->flags & MOD_USED_ONCE) && !__MOD_IN_USE(mod)) { if ((mod->flags & MOD_VISITED) && !(mod->flags & MOD_JUST_FREED)) { spin_unlock(&unload_lock); mod->flags &= ~MOD_VISITED; } else { /* check that we have permission to do this * an error is not propagated if perm fails */ if (security_ops->module_ops->delete_module(0,mod)) { spin_unlock(&unload_lock); continue; } mod->flags |= MOD_DELETED; spin_unlock(&unload_lock); free_module(mod, 1); something_changed = 1; } } else { spin_unlock(&unload_lock); } } if (something_changed) goto restart; for (mod = module_list; mod != &kernel_module; mod = mod->next) mod->flags &= ~MOD_JUST_FREED; error = 0; out: unlock_kernel(); return error; } richard. -- ----------------------------------------------------------------------- Richard Offer Technical Lead, Trust Technology, SGI "Specialization is for insects" ___________________________________________On sabatical Nov 8 -> Nov 30 _______________________________________________ linux-security-module mailing list linux-security-moduleat_private http://mail.wirex.com/mailman/listinfo/linux-security-module
This archive was generated by hypermail 2b30 : Thu Oct 18 2001 - 17:29:59 PDT