| Message ID | 20170120131000.GR3781@localhost.localdomain |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
On 20/01/17 13:10, Marcelo Ricardo Leitner wrote: > On Fri, Jan 20, 2017 at 01:01:57PM +0000, Colin King wrote: >> From: Colin Ian King <colin.king@canonical.com> >> >> The comparison on the timeout can lead to an array overrun >> read on sctp_timer_tbl because of an off-by-one error. Fix >> this by using < instead of <= and also compare to the array >> size rather than SCTP_EVENT_TIMEOUT_MAX. >> >> Fixes CoverityScan CID#1397639 ("Out-of-bounds read") >> >> Signed-off-by: Colin Ian King <colin.king@canonical.com> >> --- >> net/sctp/debug.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/net/sctp/debug.c b/net/sctp/debug.c >> index 95d7b15..e371a0d 100644 >> --- a/net/sctp/debug.c >> +++ b/net/sctp/debug.c >> @@ -166,7 +166,7 @@ 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) >> + if (id.timeout < ARRAY_SIZE(sctp_timer_tbl)) >> return sctp_timer_tbl[id.timeout]; > > The issue exists but this is not the right fix. > Issue was introduced by 7b9438de0cd4 ("sctp: add stream reconf timer") > as it introduced a new timer but didn't add the timer name here, so the > fix should (also) include: > > diff --git a/net/sctp/debug.c b/net/sctp/debug.c > index 95d7b15dad21..c5f4ed5242ac 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", > }; Ah, OK, I can add that timeout into the the table, but perhaps it's still prudent to check the index against the table size as well. > > Thanks, > Marcelo > >> 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 >>
On Fri, Jan 20, 2017 at 01:15:18PM +0000, Colin Ian King wrote: > On 20/01/17 13:10, Marcelo Ricardo Leitner wrote: > > On Fri, Jan 20, 2017 at 01:01:57PM +0000, Colin King wrote: > >> From: Colin Ian King <colin.king@canonical.com> > >> > >> The comparison on the timeout can lead to an array overrun > >> read on sctp_timer_tbl because of an off-by-one error. Fix > >> this by using < instead of <= and also compare to the array > >> size rather than SCTP_EVENT_TIMEOUT_MAX. > >> > >> Fixes CoverityScan CID#1397639 ("Out-of-bounds read") > >> > >> Signed-off-by: Colin Ian King <colin.king@canonical.com> > >> --- > >> net/sctp/debug.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/net/sctp/debug.c b/net/sctp/debug.c > >> index 95d7b15..e371a0d 100644 > >> --- a/net/sctp/debug.c > >> +++ b/net/sctp/debug.c > >> @@ -166,7 +166,7 @@ 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) > >> + if (id.timeout < ARRAY_SIZE(sctp_timer_tbl)) > >> return sctp_timer_tbl[id.timeout]; > > > > The issue exists but this is not the right fix. > > Issue was introduced by 7b9438de0cd4 ("sctp: add stream reconf timer") > > as it introduced a new timer but didn't add the timer name here, so the > > fix should (also) include: > > > > diff --git a/net/sctp/debug.c b/net/sctp/debug.c > > index 95d7b15dad21..c5f4ed5242ac 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", > > }; > > Ah, OK, I can add that timeout into the the table, but perhaps it's > still prudent to check the index against the table size as well. > Yes, and/or a: BUILD_BUG_ON(SCTP_EVENT_TIMEOUT_MAX + 1 != ARRAY_SIZE(sctp_timer_tbl)) As then we would be sure to have the list right :-) Otherwise we may not do wrong reads but may report the wrong timer name. > > > > Thanks, > > Marcelo > > > >> 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 > >> > > -- > 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 >
diff --git a/net/sctp/debug.c b/net/sctp/debug.c index 95d7b15dad21..c5f4ed5242ac 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", };