[PATCH 0/2] module: some refactoring in module_sig_check()

[PATCH 0/2] module: some refactoring in module_sig_check()

[Sergey Shtylyov]

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()

[PATCH 1/2] module: merge repetitive strings in module_sig_check()

[Sergey Shtylyov]

The 'reason' variable in module_sig_check() points to 3 strings across
the *switch* statement, all needlessly starting with the same text.  Let's
put as much of the starting text as we can into the pr_notice() call (this
includes some rewording of the 1st message) -- it saves 37 bytes of object
code (x86 gcc 10.2.1).

Signed-off-by: Sergey Shtylyov <s.shtylyov@omprussia.ru>

---
 kernel/module.c |    9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

Index: linux/kernel/module.c
===================================================================
--- linux.orig/kernel/module.c
+++ linux/kernel/module.c
@@ -2906,16 +2906,17 @@ static int module_sig_check(struct load_
 		 * enforcing, certain errors are non-fatal.
 		 */
 	case -ENODATA:
-		reason = "Loading of unsigned module";
+		reason = "no signature";
 		goto decide;
 	case -ENOPKG:
-		reason = "Loading of module with unsupported crypto";
+		reason = "unsupported crypto";
 		goto decide;
 	case -ENOKEY:
-		reason = "Loading of module with unavailable key";
+		reason = "unavailable key";
 	decide:
 		if (is_module_sig_enforced()) {
-			pr_notice("%s: %s is rejected\n", info->name, reason);
+			pr_notice("%s: loading of module with %s is rejected\n",
+				  info->name, reason);
 			return -EKEYREJECTED;
 		}
 

[PATCH 2/2] module: unindent comments in module_sig_check()

[Sergey Shtylyov]

The way the comments in the *switch* statement in module_sig_check() are
indented, it may seem they refer to the statements above them, not below.
Align the comments with the *case* and *default* labels below them, fixing
the comment style and adding article/dash, while at it...

Signed-off-by: Sergey Shtylyov <s.shtylyov@omprussia.ru>

---
 kernel/module.c |   17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

Index: linux/kernel/module.c
===================================================================
--- linux.orig/kernel/module.c
+++ linux/kernel/module.c
@@ -2901,10 +2901,11 @@ static int module_sig_check(struct load_
 		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.
-		 */
+	/*
+	 * We don't permit modules to be loaded into the trusted kernels
+	 * without a valid signature on them, but if we're not enforcing,
+	 * certain errors are non-fatal.
+	 */
 	case -ENODATA:
 		reason = "no signature";
 		goto decide;
@@ -2922,10 +2923,10 @@ static int module_sig_check(struct load_
 
 		return security_locked_down(LOCKDOWN_MODULE_SIGNATURE);
 
-		/* All other errors are fatal, including nomem, unparseable
-		 * signatures and signature check failures - even if signatures
-		 * aren't required.
-		 */
+	/*
+	 * All other errors are fatal, including nomem, unparseable signatures
+	 * and signature check failures -- even if signatures aren't required.
+	 */
 	default:
 		return err;
 	}

Re: [PATCH 0/2] module: some refactoring in module_sig_check()

[Joe Perches]

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)

Re: [PATCH 0/2] module: some refactoring in module_sig_check()

[Sergey Shtylyov]

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()

[Joe Perches]

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 <joe@perches.com>

Re: [PATCH 0/2] module: some refactoring in module_sig_check()

[Sergey Shtylyov]

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 <joe@perches.com>

   I think I'll rather use Suggested-by: where appropriate...

MBR, Sergei

Re: [PATCH 0/2] module: some refactoring in module_sig_check()

[Joe Perches]

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 <joe@perches.com>
> 
>    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()

[Jessica Yu]

+++ 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()

[Sergey Shtylyov]

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