* [PATCH] net: sctp: fix array overrun read on sctp_timer_tbl
@ 2017-01-20 13:01 Colin King
2017-01-20 13:10 ` Marcelo Ricardo Leitner
2017-01-20 16:26 ` David Miller
0 siblings, 2 replies; 8+ messages in thread
From: Colin King @ 2017-01-20 13:01 UTC (permalink / raw)
To: Vlad Yasevich, Neil Horman, David S . Miller, linux-sctp, netdev
Cc: linux-kernel
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];
return "unknown_timer";
}
--
2.10.2
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH] net: sctp: fix array overrun read on sctp_timer_tbl 2017-01-20 13:01 [PATCH] net: sctp: fix array overrun read on sctp_timer_tbl Colin King @ 2017-01-20 13:10 ` Marcelo Ricardo Leitner 2017-01-20 16:26 ` David Miller 1 sibling, 0 replies; 8+ messages in thread From: Marcelo Ricardo Leitner @ 2017-01-20 13:10 UTC (permalink / raw) To: Colin King Cc: Vlad Yasevich, Neil Horman, David S . Miller, linux-sctp, netdev, linux-kernel, lucien.xin 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", }; 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 > ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] net: sctp: fix array overrun read on sctp_timer_tbl @ 2017-01-20 13:10 ` Marcelo Ricardo Leitner 0 siblings, 0 replies; 8+ messages in thread From: Marcelo Ricardo Leitner @ 2017-01-20 13:10 UTC (permalink / raw) To: Colin King Cc: Vlad Yasevich, Neil Horman, David S . Miller, linux-sctp, netdev, linux-kernel, lucien.xin 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", }; 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 > ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] net: sctp: fix array overrun read on sctp_timer_tbl 2017-01-20 13:10 ` Marcelo Ricardo Leitner (?) @ 2017-01-20 13:15 ` Colin Ian King 2017-01-20 13:22 ` Marcelo Ricardo Leitner -1 siblings, 1 reply; 8+ messages in thread From: Colin Ian King @ 2017-01-20 13:15 UTC (permalink / raw) To: Marcelo Ricardo Leitner Cc: Vlad Yasevich, Neil Horman, David S . Miller, linux-sctp, netdev, linux-kernel, lucien.xin 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 >> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] net: sctp: fix array overrun read on sctp_timer_tbl 2017-01-20 13:15 ` Colin Ian King @ 2017-01-20 13:22 ` Marcelo Ricardo Leitner 0 siblings, 0 replies; 8+ messages in thread From: Marcelo Ricardo Leitner @ 2017-01-20 13:22 UTC (permalink / raw) To: Colin Ian King Cc: Vlad Yasevich, Neil Horman, David S . Miller, linux-sctp, netdev, linux-kernel, lucien.xin 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 > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] net: sctp: fix array overrun read on sctp_timer_tbl @ 2017-01-20 13:22 ` Marcelo Ricardo Leitner 0 siblings, 0 replies; 8+ messages in thread From: Marcelo Ricardo Leitner @ 2017-01-20 13:22 UTC (permalink / raw) To: Colin Ian King Cc: Vlad Yasevich, Neil Horman, David S . Miller, linux-sctp, netdev, linux-kernel, lucien.xin 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 > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] net: sctp: fix array overrun read on sctp_timer_tbl 2017-01-20 13:01 [PATCH] net: sctp: fix array overrun read on sctp_timer_tbl Colin King @ 2017-01-20 16:26 ` David Miller 2017-01-20 16:26 ` David Miller 1 sibling, 0 replies; 8+ messages in thread From: David Miller @ 2017-01-20 16:26 UTC (permalink / raw) To: colin.king; +Cc: vyasevich, nhorman, linux-sctp, netdev, linux-kernel From: Colin King <colin.king@canonical.com> Date: Fri, 20 Jan 2017 13:01:57 +0000 > 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> Applied. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] net: sctp: fix array overrun read on sctp_timer_tbl @ 2017-01-20 16:26 ` David Miller 0 siblings, 0 replies; 8+ messages in thread From: David Miller @ 2017-01-20 16:26 UTC (permalink / raw) To: colin.king; +Cc: vyasevich, nhorman, linux-sctp, netdev, linux-kernel From: Colin King <colin.king@canonical.com> Date: Fri, 20 Jan 2017 13:01:57 +0000 > 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> Applied. ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2017-01-20 16:26 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-01-20 13:01 [PATCH] net: sctp: fix array overrun read on sctp_timer_tbl Colin King 2017-01-20 13:10 ` Marcelo Ricardo Leitner 2017-01-20 13:10 ` Marcelo Ricardo Leitner 2017-01-20 13:15 ` Colin Ian King 2017-01-20 13:22 ` Marcelo Ricardo Leitner 2017-01-20 13:22 ` Marcelo Ricardo Leitner 2017-01-20 16:26 ` David Miller 2017-01-20 16:26 ` David Miller
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.