[PATCH] apparmor: fix off-by-one comparison on MAXMAPPED_SIG

[PATCH] apparmor: fix off-by-one comparison on MAXMAPPED_SIG

[John Johansen]

This came in yesterday, and I have verified our regression tests
were missing this and it can cause an oops. Please apply.

There is a an off-by-one comparision on sig against MAXMAPPED_SIG
that can lead to a read outside the sig_map array if sig
is MAXMAPPED_SIG. Fix this.

Verified that the check is an out of bounds case that can cause an oops.

Revised: add comparison fix to second case
Fixes: cd1dbf76b23d ("apparmor: add the ability to mediate signals")
Signed-off-by: Colin Ian King <colin.king@canonical.com>
Signed-off-by: John Johansen <john.johansen@canonical.com>
---
 security/apparmor/ipc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/security/apparmor/ipc.c b/security/apparmor/ipc.c
index 66fb9ede9447..7ca0032e7ba9 100644
--- a/security/apparmor/ipc.c
+++ b/security/apparmor/ipc.c
@@ -128,7 +128,7 @@ static inline int map_signal_num(int sig)
 		return SIGUNKNOWN;
 	else if (sig >= SIGRTMIN)
 		return sig - SIGRTMIN + 128;	/* rt sigs mapped to 128 */
-	else if (sig <= MAXMAPPED_SIG)
+	else if (sig < MAXMAPPED_SIG)
 		return sig_map[sig];
 	return SIGUNKNOWN;
 }
@@ -163,7 +163,7 @@ static void audit_signal_cb(struct audit_buffer *ab, void *va)
 			audit_signal_mask(ab, aad(sa)->denied);
 		}
 	}
-	if (aad(sa)->signal <= MAXMAPPED_SIG)
+	if (aad(sa)->signal < MAXMAPPED_SIG)
 		audit_log_format(ab, " signal=%s", sig_names[aad(sa)->signal]);
 	else
 		audit_log_format(ab, " signal=rtmin+%d",
-- 
2.11.0

Re: [PATCH] apparmor: fix off-by-one comparison on MAXMAPPED_SIG

[Linus Torvalds]

On Wed, Nov 8, 2017 at 8:09 AM, John Johansen
<john.johansen@canonical.com> wrote:
>
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> Signed-off-by: John Johansen <john.johansen@canonical.com>

This sign-off chain is odd. It implies that the patch came from Colin
King, bnut it lacks authorship attribution.

So who authored this?

If it was Colin, there should have been a

    From: Colin Ian King <colin.king@canonical.com>

at the top.

So either the authorship is broken, or the sign-off chain is. Which is it?

              Linus

Re: [PATCH] apparmor: fix off-by-one comparison on MAXMAPPED_SIG

[Linus Torvalds]

On Wed, Nov 8, 2017 at 10:53 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> So either the authorship is broken, or the sign-off chain is. Which is it?

Ahh, looking around for emails, I see that Colin sent the first
version with just one comparison, and you added the second one. So
authorship is mixed, I guess.

git doesn't support multiple authors, so I guess I'll just apply the
patch as is, and Colin misses out. Sorry, Colin.

              Linus

Re: [PATCH] apparmor: fix off-by-one comparison on MAXMAPPED_SIG

[John Johansen]

On 11/08/2017 10:53 AM, Linus Torvalds wrote:
> On Wed, Nov 8, 2017 at 8:09 AM, John Johansen
> <john.johansen@canonical.com> wrote:
>>
>> Signed-off-by: Colin Ian King <colin.king@canonical.com>
>> Signed-off-by: John Johansen <john.johansen@canonical.com>
> 
> This sign-off chain is odd. It implies that the patch came from Colin
> King, bnut it lacks authorship attribution.
> 
> So who authored this?
> 
> If it was Colin, there should have been a
> 
>     From: Colin Ian King <colin.king@canonical.com>
> 
> at the top.
> 
> So either the authorship is broken, or the sign-off chain is. Which is it?
> 
The initial patch is from Colin it covered the 1st of the 2 cases. I edited the
patch, adding the second case and added a revision note then I messed up the mail.
Rushing to get this out (I dumped it into the regular mail client instead of
using git mail).

I should have just sent the request pull

The following changes since commit 8b50f0a6261b3b6e6c7ef9febe7ab8a973243ba3:

  apparmor: fix off-by-one comparison on MAXMAPPED_SIG (2017-11-08 07:49:53 -0800)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/jj/linux-apparmor aa-for-up

for you to fetch changes up to 8b50f0a6261b3b6e6c7ef9febe7ab8a973243ba3:

  apparmor: fix off-by-one comparison on MAXMAPPED_SIG (2017-11-08 07:49:53 -0800)

----------------------------------------------------------------

Re: [PATCH] apparmor: fix off-by-one comparison on MAXMAPPED_SIG

[John Johansen]

On 11/08/2017 10:53 AM, Linus Torvalds wrote:
> On Wed, Nov 8, 2017 at 8:09 AM, John Johansen
> <john.johansen@canonical.com> wrote:
>>
>> Signed-off-by: Colin Ian King <colin.king@canonical.com>
>> Signed-off-by: John Johansen <john.johansen@canonical.com>
> 
> This sign-off chain is odd. It implies that the patch came from Colin
> King, bnut it lacks authorship attribution.
> 
> So who authored this?
> 
> If it was Colin, there should have been a
> 
>     From: Colin Ian King <colin.king@canonical.com>
> 
> at the top.
> 
> So either the authorship is broken, or the sign-off chain is. Which is it?
> 

gah, and there I go again, the correct pull


The following changes since commit 0b07194bb55ed836c2cc7c22e866b87a14681984:

  Linux 4.14-rc7 (2017-10-29 13:58:38 -0700)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/jj/linux-apparmor aa-for-up

for you to fetch changes up to 8b50f0a6261b3b6e6c7ef9febe7ab8a973243ba3:

  apparmor: fix off-by-one comparison on MAXMAPPED_SIG (2017-11-08 07:49:53 -0800)

----------------------------------------------------------------
Colin Ian King (1):
      apparmor: fix off-by-one comparison on MAXMAPPED_SIG

 security/apparmor/ipc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Re: [PATCH] apparmor: fix off-by-one comparison on MAXMAPPED_SIG

[John Johansen]

On 08/22/2017 05:58 PM, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
> 
> There is a an off-by-one comparision on sig against MAXMAPPED_SIG
> that can lead to a read outside the sig_map array if sig
> is MAXMAPPED_SIG. Fix this.
> 
> Detected with cppcheck:
> "Either the condition 'sig<=35' is redundant or the array 'sig_map[35]'
> is accessed at index 35, which is out of bounds."
> 
> Fixes: c6bf1adaecaa ("apparmor: add the ability to mediate signals")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>  security/apparmor/ipc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/security/apparmor/ipc.c b/security/apparmor/ipc.c
> index 66fb9ede9447..5091c78062e4 100644
> --- a/security/apparmor/ipc.c
> +++ b/security/apparmor/ipc.c
> @@ -128,7 +128,7 @@ static inline int map_signal_num(int sig)
>  		return SIGUNKNOWN;
>  	else if (sig >= SIGRTMIN)
>  		return sig - SIGRTMIN + 128;	/* rt sigs mapped to 128 */
> -	else if (sig <= MAXMAPPED_SIG)
> +	else if (sig < MAXMAPPED_SIG)
>  		return sig_map[sig];
>  	return SIGUNKNOWN;
>  }
> 

I've pulled this in to apparmor-next

[PATCH] apparmor: fix off-by-one comparison on MAXMAPPED_SIG

[Colin King]

From: Colin Ian King <colin.king@canonical.com>

There is a an off-by-one comparision on sig against MAXMAPPED_SIG
that can lead to a read outside the sig_map array if sig
is MAXMAPPED_SIG. Fix this.

Detected with cppcheck:
"Either the condition 'sig<=35' is redundant or the array 'sig_map[35]'
is accessed at index 35, which is out of bounds."

Fixes: c6bf1adaecaa ("apparmor: add the ability to mediate signals")
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
 security/apparmor/ipc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/security/apparmor/ipc.c b/security/apparmor/ipc.c
index 66fb9ede9447..5091c78062e4 100644
--- a/security/apparmor/ipc.c
+++ b/security/apparmor/ipc.c
@@ -128,7 +128,7 @@ static inline int map_signal_num(int sig)
 		return SIGUNKNOWN;
 	else if (sig >= SIGRTMIN)
 		return sig - SIGRTMIN + 128;	/* rt sigs mapped to 128 */
-	else if (sig <= MAXMAPPED_SIG)
+	else if (sig < MAXMAPPED_SIG)
 		return sig_map[sig];
 	return SIGUNKNOWN;
 }
-- 
2.14.1