Re: [PATCH 0/2] module: some refactoring in module_sig_check()
Hello! On 10/22/20 6:09 PM, Jessica Yu wrote: >> Here are 2 patches against the 'modules-next' branch of Jessica Yu's >> 'linux.git' repo. >> I'm doing some little refactoring in module_sig_check()... >> >> [1/2] module: merge repetitive strings in module_sig_check() >> [2/2] module: unindent comments in module_sig_check() > > Hi Sergey, > > I'm fine with these patches, but are you still planning on sending a > v2 based on Joe Perches' suggestions? Yes, I'm going to address his feedback, as soon as I have a time. > Thanks! > > Jessica MBR, Sergei
Re: [PATCH 0/2] module: some refactoring in module_sig_check()
+++ Sergey Shtylyov [13/10/20 23:32 +0300]: Here are 2 patches against the 'modules-next' branch of Jessica Yu's 'linux.git' repo. I'm doing some little refactoring in module_sig_check()... [1/2] module: merge repetitive strings in module_sig_check() [2/2] module: unindent comments in module_sig_check() Hi Sergey, I'm fine with these patches, but are you still planning on sending a v2 based on Joe Perches' suggestions? Thanks! Jessica
Re: [PATCH 0/2] module: some refactoring in module_sig_check()
On Wed, 2020-10-14 at 22:44 +0300, Sergey Shtylyov wrote: > On 10/14/20 11:35 AM, Joe Perches wrote: > > [...] > > > > > Here are 2 patches against the 'modules-next' branch of Jessica Yu's > > > > > 'linux.git' repo. > > > > > I'm doing some little refactoring in module_sig_check()... > > > > > > > > > > [1/2] module: merge repetitive strings in module_sig_check() > > > > > [2/2] module: unindent comments in module_sig_check() > > > > > > > > I think this code is rather cryptic and could be made clearer. > > > > > > > > How about: > > > > --- > > > > kernel/module.c | 51 > > > > ++- > > > > 1 file changed, 26 insertions(+), 25 deletions(-) > > > > > > Looks good. Do you want to post complete patch(es)? :-) > > > > I don't like posting actual patches on top of other people. > > It's a complete and compilable diff, it's just unsigned. > >It does too many things simultaneously, I'll need to decompose it... > > > If you want a Signed-off-by: here's one: > > > > Signed-off-by: Joe Perches > >I think I'll rather use Suggested-by: where appropriate... I don't think it does too many things and I'm not a big fan of micro-patches, but no worries, do what you want to it. I just thought the code poor and a bit unreadable given the internal goto in a switch/case block.
Re: [PATCH 0/2] module: some refactoring in module_sig_check()
On 10/14/20 11:35 AM, Joe Perches wrote: [...] Here are 2 patches against the 'modules-next' branch of Jessica Yu's 'linux.git' repo. I'm doing some little refactoring in module_sig_check()... [1/2] module: merge repetitive strings in module_sig_check() [2/2] module: unindent comments in module_sig_check() >>> >>> I think this code is rather cryptic and could be made clearer. >>> >>> How about: >>> --- >>> kernel/module.c | 51 ++- >>> 1 file changed, 26 insertions(+), 25 deletions(-) >> >> Looks good. Do you want to post complete patch(es)? :-) > > I don't like posting actual patches on top of other people. > It's a complete and compilable diff, it's just unsigned. It does too many things simultaneously, I'll need to decompose it... > If you want a Signed-off-by: here's one: > > Signed-off-by: Joe Perches I think I'll rather use Suggested-by: where appropriate... MBR, Sergei
Re: [PATCH 0/2] module: some refactoring in module_sig_check()
On Wed, 2020-10-14 at 11:18 +0300, Sergey Shtylyov wrote: > Hello! > > On 14.10.2020 1:44, Joe Perches wrote: > > > > Here are 2 patches against the 'modules-next' branch of Jessica Yu's > > > 'linux.git' repo. > > > I'm doing some little refactoring in module_sig_check()... > > > > > > [1/2] module: merge repetitive strings in module_sig_check() > > > [2/2] module: unindent comments in module_sig_check() > > > > I think this code is rather cryptic and could be made clearer. > > > > How about: > > --- > > kernel/module.c | 51 ++- > > 1 file changed, 26 insertions(+), 25 deletions(-) > > Looks good. Do you want to post complete patch(es)? :-) I don't like posting actual patches on top of other people. It's a complete and compilable diff, it's just unsigned. If you want a Signed-off-by: here's one: Signed-off-by: Joe Perches
Re: [PATCH 0/2] module: some refactoring in module_sig_check()
Hello! On 14.10.2020 1:44, Joe Perches wrote: Here are 2 patches against the 'modules-next' branch of Jessica Yu's 'linux.git' repo. I'm doing some little refactoring in module_sig_check()... [1/2] module: merge repetitive strings in module_sig_check() [2/2] module: unindent comments in module_sig_check() I think this code is rather cryptic and could be made clearer. How about: --- kernel/module.c | 51 ++- 1 file changed, 26 insertions(+), 25 deletions(-) Looks good. Do you want to post complete patch(es)? :-) [...] MBR, Sergei
Re: [PATCH 0/2] module: some refactoring in module_sig_check()
On Tue, 2020-10-13 at 23:32 +0300, Sergey Shtylyov wrote:
> Here are 2 patches against the 'modules-next' branch of Jessica Yu's
> 'linux.git' repo.
> I'm doing some little refactoring in module_sig_check()...
>
> [1/2] module: merge repetitive strings in module_sig_check()
> [2/2] module: unindent comments in module_sig_check()
I think this code is rather cryptic and could be made clearer.
How about:
---
kernel/module.c | 51 ++-
1 file changed, 26 insertions(+), 25 deletions(-)
diff --git a/kernel/module.c b/kernel/module.c
index c075a18103fb..98b3af96a5bd 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2884,46 +2884,47 @@ static int module_sig_check(struct load_info *info, int
flags)
* Require flags == 0, as a module with version information
* removed is no longer the module that was signed
*/
- if (flags == 0 &&
- info->len > markerlen &&
- memcmp(mod + info->len - markerlen, MODULE_SIG_STRING, markerlen)
== 0) {
+ if (flags == 0 && info->len > markerlen &&
+ !memcmp(mod + info->len - markerlen, MODULE_SIG_STRING, markerlen))
{
/* We truncate the module to discard the signature */
info->len -= markerlen;
err = mod_verify_sig(mod, info);
+ if (!err) {
+ info->sig_ok = true;
+ return 0;
+ }
}
+ /*
+* We don't permit modules to be loaded into trusted kernels
+* without a valid signature on them, but if we're not enforcing,
+* some errors are non-fatal.
+*/
switch (err) {
- case 0:
- info->sig_ok = true;
- return 0;
-
- /* We don't permit modules to be loaded into trusted kernels
-* without a valid signature on them, but if we're not
-* enforcing, certain errors are non-fatal.
-*/
case -ENODATA:
- reason = "Loading of unsigned module";
- goto decide;
+ reason = "unsigned module";
+ break;
case -ENOPKG:
- reason = "Loading of module with unsupported crypto";
- goto decide;
+ reason = "module with unsupported crypto";
+ break;
case -ENOKEY:
- reason = "Loading of module with unavailable key";
- decide:
- if (is_module_sig_enforced()) {
- pr_notice("%s: %s is rejected\n", info->name, reason);
- return -EKEYREJECTED;
- }
-
- return security_locked_down(LOCKDOWN_MODULE_SIGNATURE);
-
+ reason = "module with unavailable key";
+ break;
+ default:
/* All other errors are fatal, including nomem, unparseable
* signatures and signature check failures - even if signatures
* aren't required.
*/
- default:
return err;
}
+
+ if (is_module_sig_enforced()) {
+ pr_notice("%s: loading of %s is rejected\n",
+ info->name, reason);
+ return -EKEYREJECTED;
+ }
+
+ return security_locked_down(LOCKDOWN_MODULE_SIGNATURE);
}
#else /* !CONFIG_MODULE_SIG */
static int module_sig_check(struct load_info *info, int flags)
[PATCH 0/2] module: some refactoring in module_sig_check()
Here are 2 patches against the 'modules-next' branch of Jessica Yu's 'linux.git' repo. I'm doing some little refactoring in module_sig_check()... [1/2] module: merge repetitive strings in module_sig_check() [2/2] module: unindent comments in module_sig_check()
