| Message ID | 1412888133-833-3-git-send-email-dborkman@redhat.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
On Thu, Oct 09, 2014 at 10:55:32PM +0200, Daniel Borkmann wrote: > When receiving a e.g. semi-good formed connection scan in the > form of ... > > -------------- INIT[ASCONF; ASCONF_ACK] -------------> > <----------- INIT-ACK[ASCONF; ASCONF_ACK] ------------ > -------------------- COOKIE-ECHO --------------------> > <-------------------- COOKIE-ACK --------------------- > ---------------- ASCONF_a; ASCONF_b -----------------> > > ... where ASCONF_a equals ASCONF_b chunk (at least both serials > need to be equal), we panic an SCTP server! > > The problem is that good-formed ASCONF chunks that we reply with > ASCONF_ACK chunks are cached per serial. Thus, when we receive a > same ASCONF chunk twice (e.g. through a lost ASCONF_ACK), we do > not need to process them again on the server side (that was the > idea, also proposed in the RFC). Instead, we know it was cached > and we just resend the cached chunk instead. So far, so good. > > Where things get nasty is in SCTP's side effect interpreter, that > is, sctp_cmd_interpreter(): > > While incoming ASCONF_a (chunk = event_arg) is being marked > !end_of_packet and !singleton, and we have an association context, > we do not flush the outqueue the first time after processing the > ASCONF_ACK singleton chunk via SCTP_CMD_REPLY. Instead, we keep it > queued up, although we set local_cork to 1. Commit 2e3216cd54b1 > changed the precedence, so that as long as we get bundled, incoming > chunks we try possible bundling on outgoing queue as well. Before > this commit, we would just flush the output queue. > > Now, while ASCONF_a's ASCONF_ACK sits in the corked outq, we > continue to process the same ASCONF_b chunk from the packet. As > we have cached the previous ASCONF_ACK, we find it, grab it and > do another SCTP_CMD_REPLY command on it. So, effectively, we rip > the chunk->list pointers and requeue the same ASCONF_ACK chunk > another time. Since we process ASCONF_b, it's correctly marked > with end_of_packet and we enforce an uncork, and thus flush, thus > crashing the kernel. > > Fix it by testing if the ASCONF_ACK is currently pending and if > that is the case, do not requeue it. When flushing the output > queue we may relink the chunk for preparing an outgoing packet, > but eventually unlink it when it's copied into the skb right > before transmission. > > Joint work with Vlad Yasevich. > > Fixes: 2e3216cd54b1 ("sctp: Follow security requirement of responding with 1 packet") > Signed-off-by: Daniel Borkmann <dborkman@redhat.com> > Signed-off-by: Vlad Yasevich <vyasevich@gmail.com> > --- > include/net/sctp/sctp.h | 5 +++++ > net/sctp/associola.c | 2 ++ > 2 files changed, 7 insertions(+) > > diff --git a/include/net/sctp/sctp.h b/include/net/sctp/sctp.h > index 9fbd856..856f01c 100644 > --- a/include/net/sctp/sctp.h > +++ b/include/net/sctp/sctp.h > @@ -426,6 +426,11 @@ static inline void sctp_assoc_pending_pmtu(struct sock *sk, struct sctp_associat > asoc->pmtu_pending = 0; > } > > +static inline bool sctp_chunk_pending(const struct sctp_chunk *chunk) > +{ > + return !list_empty(&chunk->list); > +} > + > /* Walk through a list of TLV parameters. Don't trust the > * individual parameter lengths and instead depend on > * the chunk length to indicate when to stop. Make sure > diff --git a/net/sctp/associola.c b/net/sctp/associola.c > index a88b852..f791edd 100644 > --- a/net/sctp/associola.c > +++ b/net/sctp/associola.c > @@ -1668,6 +1668,8 @@ struct sctp_chunk *sctp_assoc_lookup_asconf_ack( > * ack chunk whose serial number matches that of the request. > */ > list_for_each_entry(ack, &asoc->asconf_ack_list, transmitted_list) { > + if (sctp_chunk_pending(ack)) > + continue; > if (ack->subh.addip_hdr->serial == serial) { > sctp_chunk_hold(ack); > return ack; > -- > 1.7.11.7 > > -- > 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 > Is it worth adding a WARN_ON, to indicate that two ASCONF chunks have been received with duplicate serials? Neil -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/10/2014 05:39 PM, Neil Horman wrote: ... > Is it worth adding a WARN_ON, to indicate that two ASCONF chunks have been > received with duplicate serials? Don't think so, as this would be triggerable from outside. Thanks, Daniel -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Oct 11, 2014 at 12:02:31AM +0200, Daniel Borkmann wrote: > On 10/10/2014 05:39 PM, Neil Horman wrote: > ... > >Is it worth adding a WARN_ON, to indicate that two ASCONF chunks have been > >received with duplicate serials? > > Don't think so, as this would be triggerable from outside. > WARN_ON_ONCE then, per serial number? Neil > Thanks, > Daniel > -- > To unsubscribe from this list: send the line "unsubscribe netdev" 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 netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
I prefer we stay away from WARN-ONs, but pr_debug might be useful. -vlad On Sat, Oct 11, 2014 at 9:42 PM, Neil Horman <nhorman@tuxdriver.com> wrote: > On Sat, Oct 11, 2014 at 12:02:31AM +0200, Daniel Borkmann wrote: >> On 10/10/2014 05:39 PM, Neil Horman wrote: >> ... >> >Is it worth adding a WARN_ON, to indicate that two ASCONF chunks have been >> >received with duplicate serials? >> >> Don't think so, as this would be triggerable from outside. >> > WARN_ON_ONCE then, per serial number? > Neil > >> Thanks, >> Daniel >> -- >> To unsubscribe from this list: send the line "unsubscribe netdev" 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 netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/12/2014 03:42 AM, Neil Horman wrote: > On Sat, Oct 11, 2014 at 12:02:31AM +0200, Daniel Borkmann wrote: >> On 10/10/2014 05:39 PM, Neil Horman wrote: >> ... >>> Is it worth adding a WARN_ON, to indicate that two ASCONF chunks have been >>> received with duplicate serials? >> >> Don't think so, as this would be triggerable from outside. >> > WARN_ON_ONCE then, per serial number? Sorry, but no. If someone seriously runs that in production and it triggers a WARN() from outside, admins will start sending us bug reports that apparently something with the kernel code is wrong. WARN() should only be used if we have some *internal* unexpected bug, but can still fail gracefully. This would neither be an actual code bug nor would it be an internally triggered one, plus we add unnecessary complexity to the code. Similarly, for those reasons we don't WARN() and throw a stack trace when we receive, say, an skb of invalid length elsewhere. I'd also like to avoid any additional pr_debug(). I don't think people enable them in production, and if they really do, it's too late anyway as we already have received this chunk. If anything, I'd rather like to see debugging code further removed as we have already different facilities in the kernel for runtime debugging that are much more powerful. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Oct 13, 2014 at 01:25:11AM +0200, Daniel Borkmann wrote: > On 10/12/2014 03:42 AM, Neil Horman wrote: > >On Sat, Oct 11, 2014 at 12:02:31AM +0200, Daniel Borkmann wrote: > >>On 10/10/2014 05:39 PM, Neil Horman wrote: > >>... > >>>Is it worth adding a WARN_ON, to indicate that two ASCONF chunks have been > >>>received with duplicate serials? > >> > >>Don't think so, as this would be triggerable from outside. > >> > >WARN_ON_ONCE then, per serial number? > > Sorry, but no. If someone seriously runs that in production and it > triggers a WARN() from outside, admins will start sending us bug > reports that apparently something with the kernel code is wrong. > > WARN() should only be used if we have some *internal* unexpected bug, > but can still fail gracefully. This would neither be an actual code bug > nor would it be an internally triggered one, plus we add unnecessary > complexity to the code. Similarly, for those reasons we don't WARN() > and throw a stack trace when we receive, say, an skb of invalid length > elsewhere. > > I'd also like to avoid any additional pr_debug(). > > I don't think people enable them in production, and if they really do, > it's too late anyway as we already have received this chunk. If anything, > I'd rather like to see debugging code further removed as we have already > different facilities in the kernel for runtime debugging that are much > more powerful. What do you suggest then? It seems like this is a protocol error that an administrator will want to be made aware of. I'm open to other options, but just saying "no" isn't sufficient for me. Neil > -- > To unsubscribe from this list: send the line "unsubscribe netdev" 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 netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/13/2014 01:25 AM, Daniel Borkmann wrote: > On 10/12/2014 03:42 AM, Neil Horman wrote: >> On Sat, Oct 11, 2014 at 12:02:31AM +0200, Daniel Borkmann wrote: >>> On 10/10/2014 05:39 PM, Neil Horman wrote: >>> ... >>>> Is it worth adding a WARN_ON, to indicate that two ASCONF chunks have been >>>> received with duplicate serials? >>> >>> Don't think so, as this would be triggerable from outside. >>> >> WARN_ON_ONCE then, per serial number? > > Sorry, but no. If someone seriously runs that in production and it > triggers a WARN() from outside, admins will start sending us bug > reports that apparently something with the kernel code is wrong. > > WARN() should only be used if we have some *internal* unexpected bug, > but can still fail gracefully. This would neither be an actual code bug > nor would it be an internally triggered one, plus we add unnecessary > complexity to the code. Similarly, for those reasons we don't WARN() > and throw a stack trace when we receive, say, an skb of invalid length > elsewhere. > > I'd also like to avoid any additional pr_debug(). Why avoid additional pr_debugs()? The seem cheap, and in this particular case, they would be rather useful. > > I don't think people enable them in production, and if they really do, > it's too late anyway as we already have received this chunk. If anything, > I'd rather like to see debugging code further removed as we have already > different facilities in the kernel for runtime debugging that are much > more powerful. Such as? pr_debug is part of dynamic debugging which is what we want here. In prod environments, we don't want any output, but when someone needs to figure out why something doesn't work, it is very useful to have. -vlad -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/15/2014 04:58 AM, Neil Horman wrote: > On Mon, Oct 13, 2014 at 01:25:11AM +0200, Daniel Borkmann wrote: >> On 10/12/2014 03:42 AM, Neil Horman wrote: >>> On Sat, Oct 11, 2014 at 12:02:31AM +0200, Daniel Borkmann wrote: >>>> On 10/10/2014 05:39 PM, Neil Horman wrote: >>>> ... >>>>> Is it worth adding a WARN_ON, to indicate that two ASCONF chunks have been >>>>> received with duplicate serials? >>>> >>>> Don't think so, as this would be triggerable from outside. >>>> >>> WARN_ON_ONCE then, per serial number? >> >> Sorry, but no. If someone seriously runs that in production and it >> triggers a WARN() from outside, admins will start sending us bug >> reports that apparently something with the kernel code is wrong. >> >> WARN() should only be used if we have some *internal* unexpected bug, >> but can still fail gracefully. This would neither be an actual code bug >> nor would it be an internally triggered one, plus we add unnecessary >> complexity to the code. Similarly, for those reasons we don't WARN() >> and throw a stack trace when we receive, say, an skb of invalid length >> elsewhere. >> >> I'd also like to avoid any additional pr_debug(). >> >> I don't think people enable them in production, and if they really do, >> it's too late anyway as we already have received this chunk. If anything, >> I'd rather like to see debugging code further removed as we have already >> different facilities in the kernel for runtime debugging that are much >> more powerful. > > What do you suggest then? It seems like this is a protocol error that an > administrator will want to be made aware of. I'm open to other options, but > just saying "no" isn't sufficient for me. So what do you want an admin to do with this information? Currently, we don't throw WARN() stack traces or whatever on other 'malformed' inputs, we either discard them silently in SCTP or send an ABORT, for example. Do you suggest the kernel should now start throwing a WARN() on every possible skb that we discard in order to inform the admin? Given that we can handle this gracefully by basically ignoring the 2nd identical ASCONF chunk in a packet and send out a single reply, that's just handled fine. -- To unsubscribe from this list: send the line "unsubscribe netdev" 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/include/net/sctp/sctp.h b/include/net/sctp/sctp.h index 9fbd856..856f01c 100644 --- a/include/net/sctp/sctp.h +++ b/include/net/sctp/sctp.h @@ -426,6 +426,11 @@ static inline void sctp_assoc_pending_pmtu(struct sock *sk, struct sctp_associat asoc->pmtu_pending = 0; } +static inline bool sctp_chunk_pending(const struct sctp_chunk *chunk) +{ + return !list_empty(&chunk->list); +} + /* Walk through a list of TLV parameters. Don't trust the * individual parameter lengths and instead depend on * the chunk length to indicate when to stop. Make sure diff --git a/net/sctp/associola.c b/net/sctp/associola.c index a88b852..f791edd 100644 --- a/net/sctp/associola.c +++ b/net/sctp/associola.c @@ -1668,6 +1668,8 @@ struct sctp_chunk *sctp_assoc_lookup_asconf_ack( * ack chunk whose serial number matches that of the request. */ list_for_each_entry(ack, &asoc->asconf_ack_list, transmitted_list) { + if (sctp_chunk_pending(ack)) + continue; if (ack->subh.addip_hdr->serial == serial) { sctp_chunk_hold(ack); return ack;
When receiving a e.g. semi-good formed connection scan in the form of ... -------------- INIT[ASCONF; ASCONF_ACK] -------------> <----------- INIT-ACK[ASCONF; ASCONF_ACK] ------------ -------------------- COOKIE-ECHO --------------------> <-------------------- COOKIE-ACK --------------------- ---------------- ASCONF_a; ASCONF_b -----------------> ... where ASCONF_a equals ASCONF_b chunk (at least both serials need to be equal), we panic an SCTP server! The problem is that good-formed ASCONF chunks that we reply with ASCONF_ACK chunks are cached per serial. Thus, when we receive a same ASCONF chunk twice (e.g. through a lost ASCONF_ACK), we do not need to process them again on the server side (that was the idea, also proposed in the RFC). Instead, we know it was cached and we just resend the cached chunk instead. So far, so good. Where things get nasty is in SCTP's side effect interpreter, that is, sctp_cmd_interpreter(): While incoming ASCONF_a (chunk = event_arg) is being marked !end_of_packet and !singleton, and we have an association context, we do not flush the outqueue the first time after processing the ASCONF_ACK singleton chunk via SCTP_CMD_REPLY. Instead, we keep it queued up, although we set local_cork to 1. Commit 2e3216cd54b1 changed the precedence, so that as long as we get bundled, incoming chunks we try possible bundling on outgoing queue as well. Before this commit, we would just flush the output queue. Now, while ASCONF_a's ASCONF_ACK sits in the corked outq, we continue to process the same ASCONF_b chunk from the packet. As we have cached the previous ASCONF_ACK, we find it, grab it and do another SCTP_CMD_REPLY command on it. So, effectively, we rip the chunk->list pointers and requeue the same ASCONF_ACK chunk another time. Since we process ASCONF_b, it's correctly marked with end_of_packet and we enforce an uncork, and thus flush, thus crashing the kernel. Fix it by testing if the ASCONF_ACK is currently pending and if that is the case, do not requeue it. When flushing the output queue we may relink the chunk for preparing an outgoing packet, but eventually unlink it when it's copied into the skb right before transmission. Joint work with Vlad Yasevich. Fixes: 2e3216cd54b1 ("sctp: Follow security requirement of responding with 1 packet") Signed-off-by: Daniel Borkmann <dborkman@redhat.com> Signed-off-by: Vlad Yasevich <vyasevich@gmail.com> --- include/net/sctp/sctp.h | 5 +++++ net/sctp/associola.c | 2 ++ 2 files changed, 7 insertions(+)