| Message ID | 20230319212746.1783033-5-mcgrof@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | module: cleanup and call taints after is inserted | expand |
On 3/19/23 22:27, Luis Chamberlain wrote: > Move early sanity checkers for the module into a helper. > This let's us make it clear when we are working with the > local copy of the module prior to allocation. > > This produces no functional changes, it just makes subsequent > changes easier to read. > > Signed-off-by: Luis Chamberlain <mcgrof@kernel.org> > --- > kernel/module/main.c | 43 ++++++++++++++++++++++++++----------------- > 1 file changed, 26 insertions(+), 17 deletions(-) > > diff --git a/kernel/module/main.c b/kernel/module/main.c > index 427284ab31f1..933cef72ae13 100644 > --- a/kernel/module/main.c > +++ b/kernel/module/main.c > @@ -2668,6 +2668,31 @@ static int unknown_module_param_cb(char *param, char *val, const char *modname, > return 0; > } > > +/* Module within temporary copy, this doesn't do any allocation */ > +static int early_mod_check(struct load_info *info, int flags) > +{ > + int err; > + > + /* > + * Now that we know we have the correct module name, check > + * if it's blacklisted. > + */ > + if (blacklisted(info->name)) { > + pr_err("Module %s is blacklisted\n", info->name); > + return -EPERM; > + } > + > + err = rewrite_section_headers(info, flags); > + if (err) > + return err; > + > + /* Check module struct version now, before we try to use module. */ > + if (!check_modstruct_version(info, info->mod)) > + return ENOEXEC; The error value when check_modstruct_version() fails is changed in this patch from -ENOEXEC to ENOEXEC and updated back again in the next patch. It would be good to avoid introducing this temporary problem and keep the value throughout as -ENOEXEC. > + > + return 0; > +} > + > /* > * Allocate and load the module: note that size of section 0 is always > * zero, and we rely on this for optional sections. > @@ -2711,26 +2736,10 @@ static int load_module(struct load_info *info, const char __user *uargs, > if (err) > goto free_copy; > > - /* > - * Now that we know we have the correct module name, check > - * if it's blacklisted. > - */ > - if (blacklisted(info->name)) { > - err = -EPERM; > - pr_err("Module %s is blacklisted\n", info->name); > - goto free_copy; > - } > - > - err = rewrite_section_headers(info, flags); > + err = early_mod_check(info, flags); > if (err) > goto free_copy; > > - /* Check module struct version now, before we try to use module. */ > - if (!check_modstruct_version(info, info->mod)) { > - err = -ENOEXEC; Original value here. > - goto free_copy; > - } > - > /* Figure out module layout, and allocate all the memory. */ > mod = layout_and_allocate(info, flags); > if (IS_ERR(mod)) { Thanks, Petr
On Fri, Mar 24, 2023 at 02:02:06PM +0100, Petr Pavlu wrote: > On 3/19/23 22:27, Luis Chamberlain wrote: > > Move early sanity checkers for the module into a helper. > > This let's us make it clear when we are working with the > > local copy of the module prior to allocation. > > > > This produces no functional changes, it just makes subsequent > > changes easier to read. > > > > Signed-off-by: Luis Chamberlain <mcgrof@kernel.org> > > --- > > kernel/module/main.c | 43 ++++++++++++++++++++++++++----------------- > > 1 file changed, 26 insertions(+), 17 deletions(-) > > > > diff --git a/kernel/module/main.c b/kernel/module/main.c > > index 427284ab31f1..933cef72ae13 100644 > > --- a/kernel/module/main.c > > +++ b/kernel/module/main.c > > @@ -2668,6 +2668,31 @@ static int unknown_module_param_cb(char *param, char *val, const char *modname, > > return 0; > > } > > > > +/* Module within temporary copy, this doesn't do any allocation */ > > +static int early_mod_check(struct load_info *info, int flags) > > +{ > > + int err; > > + > > + /* > > + * Now that we know we have the correct module name, check > > + * if it's blacklisted. > > + */ > > + if (blacklisted(info->name)) { > > + pr_err("Module %s is blacklisted\n", info->name); > > + return -EPERM; > > + } > > + > > + err = rewrite_section_headers(info, flags); > > + if (err) > > + return err; > > + > > + /* Check module struct version now, before we try to use module. */ > > + if (!check_modstruct_version(info, info->mod)) > > + return ENOEXEC; > > The error value when check_modstruct_version() fails is changed in this patch > from -ENOEXEC to ENOEXEC and updated back again in the next patch. It would be > good to avoid introducing this temporary problem and keep the value throughout > as -ENOEXEC. Fixed, thanks. Luis
diff --git a/kernel/module/main.c b/kernel/module/main.c index 427284ab31f1..933cef72ae13 100644 --- a/kernel/module/main.c +++ b/kernel/module/main.c @@ -2668,6 +2668,31 @@ static int unknown_module_param_cb(char *param, char *val, const char *modname, return 0; } +/* Module within temporary copy, this doesn't do any allocation */ +static int early_mod_check(struct load_info *info, int flags) +{ + int err; + + /* + * Now that we know we have the correct module name, check + * if it's blacklisted. + */ + if (blacklisted(info->name)) { + pr_err("Module %s is blacklisted\n", info->name); + return -EPERM; + } + + err = rewrite_section_headers(info, flags); + if (err) + return err; + + /* Check module struct version now, before we try to use module. */ + if (!check_modstruct_version(info, info->mod)) + return ENOEXEC; + + return 0; +} + /* * Allocate and load the module: note that size of section 0 is always * zero, and we rely on this for optional sections. @@ -2711,26 +2736,10 @@ static int load_module(struct load_info *info, const char __user *uargs, if (err) goto free_copy; - /* - * Now that we know we have the correct module name, check - * if it's blacklisted. - */ - if (blacklisted(info->name)) { - err = -EPERM; - pr_err("Module %s is blacklisted\n", info->name); - goto free_copy; - } - - err = rewrite_section_headers(info, flags); + err = early_mod_check(info, flags); if (err) goto free_copy; - /* Check module struct version now, before we try to use module. */ - if (!check_modstruct_version(info, info->mod)) { - err = -ENOEXEC; - goto free_copy; - } - /* Figure out module layout, and allocate all the memory. */ mod = layout_and_allocate(info, flags); if (IS_ERR(mod)) {
Move early sanity checkers for the module into a helper. This let's us make it clear when we are working with the local copy of the module prior to allocation. This produces no functional changes, it just makes subsequent changes easier to read. Signed-off-by: Luis Chamberlain <mcgrof@kernel.org> --- kernel/module/main.c | 43 ++++++++++++++++++++++++++----------------- 1 file changed, 26 insertions(+), 17 deletions(-)