| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20131206154802.GA1426@hmsreliant.think-freely.org>
Date: Fri, 6 Dec 2013 10:48:02 -0500
From: Neil Horman <nhorman@...driver.com>
To: Vlad Yasevich <vyasevich@...il.com>
Cc: linux-sctp@...r.kernel.org, Wang Weidong <wangweidong1@...wei.com>,
David Miller <davem@...emloft.net>, netdev@...r.kernel.org
Subject: Re: [PATCH] sctp: properly latch and use autoclose value from sock
to association
On Fri, Dec 06, 2013 at 10:36:26AM -0500, Vlad Yasevich wrote:
> On 12/06/2013 10:29 AM, Neil Horman wrote:
> > Currently, sctp associations latch a sockets autoclose value to an association
> > at association init time, subject to capping constraints from the max_autoclose
> > sysctl value. This leads to an odd situation where an application may set a
> > socket level autoclose timeout, but sliently sctp will limit the autoclose
> > timeout to something less than that.
> >
> > Fix this by modifying the autoclose setsockopt function to check the limit, cap
> > it and warn the user via syslog that the timeout is capped. This will allow
> > getsockopt to return valid autoclose timeout values that reflect what subsequent
> > associations actually use.
> >
> > While were at it, also elimintate the assoc->autoclose variable, it duplicates
> > whats in the timeout array, which leads to multiple sources for the same
> > information, that may differ (as the former isn't subject to any capping). This
> > gives us the timeout information in a canonical place and saves some space in
> > the association structure as well.
> >
> > Signed-off-by: Neil Horman <nhorman@...driver.com>
> > CC: Wang Weidong <wangweidong1@...wei.com>
> > CC: David Miller <davem@...emloft.net>
> > CC: Vlad Yasevich <vyasevich@...il.com>
> > CC: netdev@...r.kernel.org
> > ---
> > include/net/sctp/structs.h | 6 ------
> > net/sctp/associola.c | 2 --
> > net/sctp/output.c | 3 ++-
> > net/sctp/sm_statefuns.c | 12 ++++++------
> > net/sctp/socket.c | 7 +++++++
> > 5 files changed, 15 insertions(+), 15 deletions(-)
> >
> > diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
> > index 2174d8d..0b069a0 100644
> > --- a/include/net/sctp/structs.h
> > +++ b/include/net/sctp/structs.h
> > @@ -1725,12 +1725,6 @@ struct sctp_association {
> > /* How many duplicated TSNs have we seen? */
> > int numduptsns;
> >
> > - /* Number of seconds of idle time before an association is closed.
> > - * In the association context, this is really used as a boolean
> > - * since the real timeout is stored in the timeouts array
> > - */
> > - __u32 autoclose;
> > -
> > /* These are to support
> > * "SCTP Extensions for Dynamic Reconfiguration of IP Addresses
> > * and Enforcement of Flow and Message Limits"
> > diff --git a/net/sctp/associola.c b/net/sctp/associola.c
> > index c9b91cb..0c9d1f1 100644
> > --- a/net/sctp/associola.c
> > +++ b/net/sctp/associola.c
> > @@ -291,8 +291,6 @@ static struct sctp_association *sctp_association_init(struct sctp_association *a
> > asoc->peer.ipv6_address = 1;
> > INIT_LIST_HEAD(&asoc->asocs);
> >
> > - asoc->autoclose = sp->autoclose;
> > -
> > asoc->default_stream = sp->default_stream;
> > asoc->default_ppid = sp->default_ppid;
> > asoc->default_flags = sp->default_flags;
> > diff --git a/net/sctp/output.c b/net/sctp/output.c
> > index e650978..c89e57d 100644
> > --- a/net/sctp/output.c
> > +++ b/net/sctp/output.c
> > @@ -580,7 +580,8 @@ int sctp_packet_transmit(struct sctp_packet *packet)
> > unsigned long timeout;
> >
> > /* Restart the AUTOCLOSE timer when sending data. */
> > - if (sctp_state(asoc, ESTABLISHED) && asoc->autoclose) {
> > + if (sctp_state(asoc, ESTABLISHED) &&
> > + asoc->timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE]) {
> > timer = &asoc->timers[SCTP_EVENT_TIMEOUT_AUTOCLOSE];
> > timeout = asoc->timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE];
> >
> > diff --git a/net/sctp/sm_statefuns.c b/net/sctp/sm_statefuns.c
> > index dfe3f36..a26065b 100644
> > --- a/net/sctp/sm_statefuns.c
> > +++ b/net/sctp/sm_statefuns.c
> > @@ -820,7 +820,7 @@ sctp_disposition_t sctp_sf_do_5_1D_ce(struct net *net,
> > SCTP_INC_STATS(net, SCTP_MIB_PASSIVEESTABS);
> > sctp_add_cmd_sf(commands, SCTP_CMD_HB_TIMERS_START, SCTP_NULL());
> >
> > - if (new_asoc->autoclose)
> > + if (new_asoc->timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE])
> > sctp_add_cmd_sf(commands, SCTP_CMD_TIMER_START,
> > SCTP_TO(SCTP_EVENT_TIMEOUT_AUTOCLOSE));
> >
> > @@ -908,7 +908,7 @@ sctp_disposition_t sctp_sf_do_5_1E_ca(struct net *net,
> > SCTP_INC_STATS(net, SCTP_MIB_CURRESTAB);
> > SCTP_INC_STATS(net, SCTP_MIB_ACTIVEESTABS);
> > sctp_add_cmd_sf(commands, SCTP_CMD_HB_TIMERS_START, SCTP_NULL());
> > - if (asoc->autoclose)
> > + if (asoc->timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE])
> > sctp_add_cmd_sf(commands, SCTP_CMD_TIMER_START,
> > SCTP_TO(SCTP_EVENT_TIMEOUT_AUTOCLOSE));
> >
> > @@ -2970,7 +2970,7 @@ sctp_disposition_t sctp_sf_eat_data_6_2(struct net *net,
> > if (chunk->chunk_hdr->flags & SCTP_DATA_SACK_IMM)
> > force = SCTP_FORCE();
> >
> > - if (asoc->autoclose) {
> > + if (asoc->timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE]) {
> > sctp_add_cmd_sf(commands, SCTP_CMD_TIMER_RESTART,
> > SCTP_TO(SCTP_EVENT_TIMEOUT_AUTOCLOSE));
> > }
> > @@ -3878,7 +3878,7 @@ sctp_disposition_t sctp_sf_eat_fwd_tsn(struct net *net,
> > SCTP_CHUNK(chunk));
> >
> > /* Count this as receiving DATA. */
> > - if (asoc->autoclose) {
> > + if (asoc->timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE]) {
> > sctp_add_cmd_sf(commands, SCTP_CMD_TIMER_RESTART,
> > SCTP_TO(SCTP_EVENT_TIMEOUT_AUTOCLOSE));
> > }
> > @@ -5267,7 +5267,7 @@ sctp_disposition_t sctp_sf_do_9_2_start_shutdown(
> > sctp_add_cmd_sf(commands, SCTP_CMD_TIMER_RESTART,
> > SCTP_TO(SCTP_EVENT_TIMEOUT_T5_SHUTDOWN_GUARD));
> >
> > - if (asoc->autoclose)
> > + if (asoc->timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE])
> > sctp_add_cmd_sf(commands, SCTP_CMD_TIMER_STOP,
> > SCTP_TO(SCTP_EVENT_TIMEOUT_AUTOCLOSE));
> >
> > @@ -5346,7 +5346,7 @@ sctp_disposition_t sctp_sf_do_9_2_shutdown_ack(
> > sctp_add_cmd_sf(commands, SCTP_CMD_TIMER_RESTART,
> > SCTP_TO(SCTP_EVENT_TIMEOUT_T2_SHUTDOWN));
> >
> > - if (asoc->autoclose)
> > + if (asoc->timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE])
> > sctp_add_cmd_sf(commands, SCTP_CMD_TIMER_STOP,
> > SCTP_TO(SCTP_EVENT_TIMEOUT_AUTOCLOSE));
> >
> > diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> > index 72046b9..9486ffd 100644
> > --- a/net/sctp/socket.c
> > +++ b/net/sctp/socket.c
> > @@ -2196,6 +2196,7 @@ static int sctp_setsockopt_autoclose(struct sock *sk, char __user *optval,
> > unsigned int optlen)
> > {
> > struct sctp_sock *sp = sctp_sk(sk);
> > + struct net *net = sock_net(sk);
> >
> > /* Applicable to UDP-style socket only */
> > if (sctp_style(sk, TCP))
> > @@ -2205,6 +2206,12 @@ static int sctp_setsockopt_autoclose(struct sock *sk, char __user *optval,
> > if (copy_from_user(&sp->autoclose, optval, optlen))
> > return -EFAULT;
> >
> > + if (sp->autoclose > net->sctp.max_autoclose) {
> > + pr_warn("Capping autoclose value %d to defined maximum of %lu\n",
> > + sp->autoclose, net->sctp.max_autoclose);
> > + sp->autoclose = net->sctp.max_autoclose;
> > + }
> > +
> > return 0;
> > }
> >
> >
>
> Now that we do this, we can do this as well:
> diff --git a/net/sctp/associola.c b/net/sctp/associola.c
> index 68a27f9..7e86957 100644
> --- a/net/sctp/associola.c
> +++ b/net/sctp/associola.c
> @@ -154,8 +154,7 @@ static struct sctp_association
> *sctp_association_init(struct sctp_association *a
>
> asoc->timeouts[SCTP_EVENT_TIMEOUT_HEARTBEAT] = 0;
> asoc->timeouts[SCTP_EVENT_TIMEOUT_SACK] = asoc->sackdelay;
> - asoc->timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE] =
> - min_t(unsigned long, sp->autoclose,
> net->sctp.max_autoclose) * HZ;
> + asoc->timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE] = sp->autoclose * HZ;
>
I actually kept that out of the patch specifically because I was concerned about
the case where we set the autoclose socket option, then reduce the max_autoclose
value. If we do this, then we have an unbounded race in which the old autoclose
value may be applied above the cap. Do you still want to put it in?
Neil
> -vlad
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
> the body of a message to majordomo@...r.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@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists