[PATCH][V2] net: sctp: fix array overrun read on sctp_timer_tbl

[PATCH][V2] net: sctp: fix array overrun read on sctp_timer_tbl

[Colin King]

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

Table sctp_timer_tbl is missing a TIMEOUT_RECONF string so
add this in. Also compare timeout with the size of the array
sctp_timer_tbl rather than SCTP_EVENT_TIMEOUT_MAX.  Also add
a build time check that SCTP_EVENT_TIMEOUT_MAX is correct
so we don't ever get this kind of mismatch between the table
and SCTP_EVENT_TIMEOUT_MAX in the future.

Kudos to Marcel Ricardo Leitner for spotting the missing string
and suggesting the build time sanity check.

Fixes CoverityScan CID#1397639 ("Out-of-bounds read")

Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
 net/sctp/debug.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/net/sctp/debug.c b/net/sctp/debug.c
index 95d7b15..2e47eb2 100644
--- a/net/sctp/debug.c
+++ b/net/sctp/debug.c
@@ -159,6 +159,7 @@ static const char *const sctp_timer_tbl[] = {
 	"TIMEOUT_T4_RTO",
 	"TIMEOUT_T5_SHUTDOWN_GUARD",
 	"TIMEOUT_HEARTBEAT",
+	"TIMEOUT_RECONF",
 	"TIMEOUT_SACK",
 	"TIMEOUT_AUTOCLOSE",
 };
@@ -166,7 +167,9 @@ static const char *const sctp_timer_tbl[] = {
 /* Lookup timer debug name. */
 const char *sctp_tname(const sctp_subtype_t id)
 {
-	if (id.timeout <= SCTP_EVENT_TIMEOUT_MAX)
+	BUILD_BUG_ON(SCTP_EVENT_TIMEOUT_MAX + 1 != ARRAY_SIZE(sctp_timer_tbl));
+
+	if (id.timeout < ARRAY_SIZE(sctp_timer_tbl))
 		return sctp_timer_tbl[id.timeout];
 	return "unknown_timer";
 }
-- 
2.10.2

Re: [PATCH][V2] net: sctp: fix array overrun read on sctp_timer_tbl

[Marcelo Ricardo Leitner]

On Fri, Jan 20, 2017 at 01:45:42PM +0000, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
> 
> Table sctp_timer_tbl is missing a TIMEOUT_RECONF string so
> add this in. Also compare timeout with the size of the array
> sctp_timer_tbl rather than SCTP_EVENT_TIMEOUT_MAX.  Also add
> a build time check that SCTP_EVENT_TIMEOUT_MAX is correct
> so we don't ever get this kind of mismatch between the table
> and SCTP_EVENT_TIMEOUT_MAX in the future.
> 
> Kudos to Marcel Ricardo Leitner for spotting the missing string
> and suggesting the build time sanity check.
> 
> Fixes CoverityScan CID#1397639 ("Out-of-bounds read")
> 
> Signed-off-by: Colin Ian King <colin.king@canonical.com>

Not sure I can add the Fixes tag for you here, but:
Fixes: 7b9438de0cd4 ("sctp: add stream reconf timer")
Acked-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>

Thanks

> ---
>  net/sctp/debug.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/net/sctp/debug.c b/net/sctp/debug.c
> index 95d7b15..2e47eb2 100644
> --- a/net/sctp/debug.c
> +++ b/net/sctp/debug.c
> @@ -159,6 +159,7 @@ static const char *const sctp_timer_tbl[] = {
>  	"TIMEOUT_T4_RTO",
>  	"TIMEOUT_T5_SHUTDOWN_GUARD",
>  	"TIMEOUT_HEARTBEAT",
> +	"TIMEOUT_RECONF",
>  	"TIMEOUT_SACK",
>  	"TIMEOUT_AUTOCLOSE",
>  };
> @@ -166,7 +167,9 @@ static const char *const sctp_timer_tbl[] = {
>  /* Lookup timer debug name. */
>  const char *sctp_tname(const sctp_subtype_t id)
>  {
> -	if (id.timeout <= SCTP_EVENT_TIMEOUT_MAX)
> +	BUILD_BUG_ON(SCTP_EVENT_TIMEOUT_MAX + 1 != ARRAY_SIZE(sctp_timer_tbl));
> +
> +	if (id.timeout < ARRAY_SIZE(sctp_timer_tbl))
>  		return sctp_timer_tbl[id.timeout];
>  	return "unknown_timer";
>  }
> -- 
> 2.10.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH][V2] net: sctp: fix array overrun read on sctp_timer_tbl

[David Miller]

From: Colin King <colin.king@canonical.com>
Date: Fri, 20 Jan 2017 13:45:42 +0000

> From: Colin Ian King <colin.king@canonical.com>
> 
> Table sctp_timer_tbl is missing a TIMEOUT_RECONF string so
> add this in. Also compare timeout with the size of the array
> sctp_timer_tbl rather than SCTP_EVENT_TIMEOUT_MAX.  Also add
> a build time check that SCTP_EVENT_TIMEOUT_MAX is correct
> so we don't ever get this kind of mismatch between the table
> and SCTP_EVENT_TIMEOUT_MAX in the future.
> 
> Kudos to Marcel Ricardo Leitner for spotting the missing string
> and suggesting the build time sanity check.
> 
> Fixes CoverityScan CID#1397639 ("Out-of-bounds read")
> 
> Signed-off-by: Colin Ian King <colin.king@canonical.com>

Well, my bad... I reverted V1, that's fine.

But this patch doesn't even compile.

In file included from ./include/uapi/linux/stddef.h:1:0,
                 from ./include/linux/stddef.h:4,
                 from ./include/uapi/linux/posix_types.h:4,
                 from ./include/uapi/linux/types.h:13,
                 from ./include/linux/types.h:5,
                 from ./include/net/sctp/sctp.h:58,
                 from net/sctp/debug.c:41:
net/sctp/debug.c: In function ‘sctp_tname’:
./include/linux/compiler.h:518:38: error: call to ‘__compiletime_assert_170’ declared with attribute error: BUILD_BUG_ON failed: SCTP_EVENT_TIMEOUT_MAX + 1 != ARRAY_SIZE(sctp_timer_tbl)
  _compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)

Re: [PATCH][V2] net: sctp: fix array overrun read on sctp_timer_tbl

[David Miller]

From: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Date: Fri, 20 Jan 2017 11:58:45 -0200

> Not sure I can add the Fixes tag for you here, but:
> Fixes: 7b9438de0cd4 ("sctp: add stream reconf timer")
> Acked-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>

FWIW, patchwork doesn't notice updated Fixes: tags like this but I
try to do so myself and the effort is definitely appreciated.

We need a V3 of this change anyways, see the build failure I reported
for V2, so Colin can add the Fixes: tag there explicitly.

Re: [PATCH][V2] net: sctp: fix array overrun read on sctp_timer_tbl

[Marcelo Ricardo Leitner]

On Fri, Jan 20, 2017 at 11:31:17AM -0500, David Miller wrote:
> From: Colin King <colin.king@canonical.com>
> Date: Fri, 20 Jan 2017 13:45:42 +0000
> 
> > From: Colin Ian King <colin.king@canonical.com>
> > 
> > Table sctp_timer_tbl is missing a TIMEOUT_RECONF string so
> > add this in. Also compare timeout with the size of the array
> > sctp_timer_tbl rather than SCTP_EVENT_TIMEOUT_MAX.  Also add
> > a build time check that SCTP_EVENT_TIMEOUT_MAX is correct
> > so we don't ever get this kind of mismatch between the table
> > and SCTP_EVENT_TIMEOUT_MAX in the future.
> > 
> > Kudos to Marcel Ricardo Leitner for spotting the missing string
> > and suggesting the build time sanity check.
> > 
> > Fixes CoverityScan CID#1397639 ("Out-of-bounds read")
> > 
> > Signed-off-by: Colin Ian King <colin.king@canonical.com>
> 
> Well, my bad... I reverted V1, that's fine.
> 
> But this patch doesn't even compile.
> 
> In file included from ./include/uapi/linux/stddef.h:1:0,
>                  from ./include/linux/stddef.h:4,
>                  from ./include/uapi/linux/posix_types.h:4,
>                  from ./include/uapi/linux/types.h:13,
>                  from ./include/linux/types.h:5,
>                  from ./include/net/sctp/sctp.h:58,
>                  from net/sctp/debug.c:41:
> net/sctp/debug.c: In function ‘sctp_tname’:
> ./include/linux/compiler.h:518:38: error: call to ‘__compiletime_assert_170’ declared with attribute error: BUILD_BUG_ON failed: SCTP_EVENT_TIMEOUT_MAX + 1 != ARRAY_SIZE(sctp_timer_tbl)
>   _compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)

Seems you applied it on net tree, but commit that introduced the issue
(7b9438de0cd4) is still only on net-next.

I build-tested it here before acking, it worked, on top of
4567d686f5c6d955e57a3afa1741944c1e7f4033.

Colin, please respin the patch.. add the Fixes tag, fix the missing 'o'
in my name on the changelog :-) and tag the patch as net-next tree too.

Thanks,
Marcelo

Re: [PATCH][V2] net: sctp: fix array overrun read on sctp_timer_tbl

[marcelo.leitner]

On Fri, Jan 20, 2017 at 11:32:09AM -0500, David Miller wrote:
> From: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
> Date: Fri, 20 Jan 2017 11:58:45 -0200
> 
> > Not sure I can add the Fixes tag for you here, but:
> > Fixes: 7b9438de0cd4 ("sctp: add stream reconf timer")
> > Acked-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
> 
> FWIW, patchwork doesn't notice updated Fixes: tags like this but I
> try to do so myself and the effort is definitely appreciated.

Ok, good to know, thanks.

> 
> We need a V3 of this change anyways, see the build failure I reported
> for V2, so Colin can add the Fixes: tag there explicitly.
> 

Replied this one in there.

  Marcelo

Re: [PATCH][V2] net: sctp: fix array overrun read on sctp_timer_tbl

[David Miller]

From: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Date: Fri, 20 Jan 2017 14:48:20 -0200

> Seems you applied it on net tree, but commit that introduced the issue
> (7b9438de0cd4) is still only on net-next.

That explains everything.

> I build-tested it here before acking, it worked, on top of
> 4567d686f5c6d955e57a3afa1741944c1e7f4033.
> 
> Colin, please respin the patch.. add the Fixes tag, fix the missing 'o'
> in my name on the changelog :-) and tag the patch as net-next tree too.

Indeed, thanks.