netdev - RE: [PATCHv2 net-next 2/5] sctp: add pf_expose per netns and sock and asoc

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <dbdb13bb2b584590b793e9a7e9b6de64@AcuMS.aculab.com>
Date:   Tue, 22 Oct 2019 11:29:09 +0000
From:   David Laight <David.Laight@...LAB.COM>
To:     'Xin Long' <lucien.xin@...il.com>
CC:     network dev <netdev@...r.kernel.org>,
        "linux-sctp@...r.kernel.org" <linux-sctp@...r.kernel.org>,
        Marcelo Ricardo Leitner <marcelo.leitner@...il.com>,
        Neil Horman <nhorman@...driver.com>,
        "davem@...emloft.net" <davem@...emloft.net>
Subject: RE: [PATCHv2 net-next 2/5] sctp: add pf_expose per netns and sock and
 asoc


From: Xin Long <lucien.xin@...il.com>
> Sent: 19 October 2019 09:45
> On Fri, Oct 18, 2019 at 11:34 PM David Laight <David.Laight@...lab.com> wrote:
> >
> > From: Xin Long
> > > Sent: 08 October 2019 12:25
> > > As said in rfc7829, section 3, point 12:
> > >
> > >   The SCTP stack SHOULD expose the PF state of its destination
> > >   addresses to the ULP as well as provide the means to notify the
> > >   ULP of state transitions of its destination addresses from
> > >   active to PF, and vice versa.  However, it is recommended that
> > >   an SCTP stack implementing SCTP-PF also allows for the ULP to be
> > >   kept ignorant of the PF state of its destinations and the
> > >   associated state transitions, thus allowing for retention of the
> > >   simpler state transition model of [RFC4960] in the ULP.
> > >
> > > Not only does it allow to expose the PF state to ULP, but also
> > > allow to ignore sctp-pf to ULP.
> > >
> > > So this patch is to add pf_expose per netns, sock and asoc. And in
> > > sctp_assoc_control_transport(), ulp_notify will be set to false if
> > > asoc->expose is not set.
> > >
> > > It also allows a user to change pf_expose per netns by sysctl, and
> > > pf_expose per sock and asoc will be initialized with it.
> > >
> > > Note that pf_expose also works for SCTP_GET_PEER_ADDR_INFO sockopt,
> > > to not allow a user to query the state of a sctp-pf peer address
> > > when pf_expose is not enabled, as said in section 7.3.
> > ...
> > > index 08d14d8..a303011 100644
> > > --- a/net/sctp/protocol.c
> > > +++ b/net/sctp/protocol.c
> > > @@ -1220,6 +1220,9 @@ static int __net_init sctp_defaults_init(struct net *net)
> > >       /* Enable pf state by default */
> > >       net->sctp.pf_enable = 1;
> > >
> > > +     /* Enable pf state exposure by default */
> > > +     net->sctp.pf_expose = 1;
> > > +
> >
> > For compatibility with existing applications pf_expose MUST default to 0.
> > I'm not even sure it makes sense to have a sysctl for it.
> You're reivewing v2, pls go and check v3 where it's:
> 
> net->sctp.pf_expose = SCTP_PF_EXPOSE_UNUSED

I'll dig out that tri-state logic again later.

> > ...
> > > @@ -5521,8 +5522,15 @@ static int sctp_getsockopt_peer_addr_info(struct sock *sk, int len,
> > >
> > >       transport = sctp_addr_id2transport(sk, &pinfo.spinfo_address,
> > >                                          pinfo.spinfo_assoc_id);
> > > -     if (!transport)
> > > -             return -EINVAL;
> > > +     if (!transport) {
> > > +             retval = -EINVAL;
> > > +             goto out;
> > > +     }
> > > +
> > > +     if (transport->state == SCTP_PF && !transport->asoc->pf_expose) {
> > > +             retval = -EACCES;
> > > +             goto out;
> > > +     }
> >
> > Ugg...
> > To avoid reporting the unexpected 'SCTP_PF' state you probable need
> > to lie about the state (probably reporting 'working' - or whatever state
> > it would be in if PF detection wasn't enabled.
> return EACCES is from RFC. see v3 where it's become:
> 
> +       if (transport->state == SCTP_PF &&
> +           transport->asoc->pf_expose == SCTP_PF_EXPOSE_DISABLE) {
> +               retval = -EACCES;
> +               goto out;
> +       }
> 
> no more compatibility issue.

Hmmm....
Never mind what the RFC says about returning EACCESS, that
is still an API change.

> > > --- a/net/sctp/sysctl.c
> > > +++ b/net/sctp/sysctl.c
> > > @@ -318,6 +318,13 @@ static struct ctl_table sctp_net_table[] = {
> > >               .mode           = 0644,
> > >               .proc_handler   = proc_dointvec,
> > >       },
> > > +     {
> > > +             .procname       = "pf_expose",
> > > +             .data           = &init_net.sctp.pf_expose,
> > > +             .maxlen         = sizeof(int),
> > > +             .mode           = 0644,
> > > +             .proc_handler   = proc_dointvec,
> > > +     },
> >
> > Setting this will break existing applications.
> > So I don't think the default should be settable.
> If the user sets this new sysctl, he must have realized what's going to happen.
> I don't think this will cause "compatibility issue".

The problem is that support is application dependant, not system dependant.
All it takes is a distro to decide to default to enabling it and all old apps break.

Given the application has to enable other things there is no reason not to
require this to be enabled by every application that wants to see the events (etc).

Note that this is different from doing the protocol part of PF - which is likely
to help applications when the 'primary' path is dodgy.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)