2017-08-22 21:58:36

by Colin King

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

From: Colin Ian King <[email protected]>

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 <[email protected]>
---
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


2017-09-25 14:21:40

by John Johansen

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

On 08/22/2017 05:58 PM, Colin King wrote:
> From: Colin Ian King <[email protected]>
>
> 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 <[email protected]>
> ---
> 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

2017-11-08 19:55:46

by John Johansen

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

On 11/08/2017 10:53 AM, Linus Torvalds wrote:
> On Wed, Nov 8, 2017 at 8:09 AM, John Johansen
> <[email protected]> wrote:
>>
>> Signed-off-by: Colin Ian King <[email protected]>
>> Signed-off-by: John Johansen <[email protected]>
>
> 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 <[email protected]>
>
> 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(-)

From 1583526318888283918@xxx Wed Nov 08 19:12:59 +0000 2017
X-GM-THRID: 1583514887991701392
X-Gmail-Labels: Inbox,Category Forums,HistoricalUnread

2017-11-08 19:12:59

by John Johansen

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

On 11/08/2017 10:53 AM, Linus Torvalds wrote:
> On Wed, Nov 8, 2017 at 8:09 AM, John Johansen
> <[email protected]> wrote:
>>
>> Signed-off-by: Colin Ian King <[email protected]>
>> Signed-off-by: John Johansen <[email protected]>
>
> 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 <[email protected]>
>
> 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)

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


From 1583525322632093446@xxx Wed Nov 08 18:57:09 +0000 2017
X-GM-THRID: 1583514887991701392
X-Gmail-Labels: Inbox,Category Forums,HistoricalUnread

2017-11-08 18:57:09

by Linus Torvalds

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

On Wed, Nov 8, 2017 at 10:53 AM, Linus Torvalds
<[email protected]> 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

From 1583525180839719555@xxx Wed Nov 08 18:54:54 +0000 2017
X-GM-THRID: 1583514887991701392
X-Gmail-Labels: Inbox,Category Forums,HistoricalUnread

2017-11-08 18:54:54

by Linus Torvalds

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

On Wed, Nov 8, 2017 at 8:09 AM, John Johansen
<[email protected]> wrote:
>
> Signed-off-by: Colin Ian King <[email protected]>
> Signed-off-by: John Johansen <[email protected]>

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 <[email protected]>

at the top.

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

Linus

From 1583514887991701392@xxx Wed Nov 08 16:11:18 +0000 2017
X-GM-THRID: 1583514887991701392
X-Gmail-Labels: Inbox,Category Forums,HistoricalUnread