netdev - RE: [PATCH net-next 5/5] sctp: add spt_pathcpthld in struct sctp

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] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 11 Sep 2019 09:03:57 +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: [PATCH net-next 5/5] sctp: add spt_pathcpthld in struct
 sctp_paddrthlds

From: Xin Long [mailto:lucien.xin@...il.com]
> Sent: 11 September 2019 09:52
> On Tue, Sep 10, 2019 at 9:19 PM David Laight <David.Laight@...lab.com> wrote:
> >
> > From: Xin Long
> > > Sent: 09 September 2019 08:57
> > > Section 7.2 of rfc7829: "Peer Address Thresholds (SCTP_PEER_ADDR_THLDS)
> > > Socket Option" extends 'struct sctp_paddrthlds' with 'spt_pathcpthld'
> > > added to allow a user to change ps_retrans per sock/asoc/transport, as
> > > other 2 paddrthlds: pf_retrans, pathmaxrxt.
> > >
> > > Note that ps_retrans is not allowed to be greater than pf_retrans.
> > >
> > > Signed-off-by: Xin Long <lucien.xin@...il.com>
> > > ---
> > >  include/uapi/linux/sctp.h |  1 +
> > >  net/sctp/socket.c         | 10 ++++++++++
> > >  2 files changed, 11 insertions(+)
> > >
> > > diff --git a/include/uapi/linux/sctp.h b/include/uapi/linux/sctp.h
> > > index a15cc28..dfd81e1 100644
> > > --- a/include/uapi/linux/sctp.h
> > > +++ b/include/uapi/linux/sctp.h
> > > @@ -1069,6 +1069,7 @@ struct sctp_paddrthlds {
> > >       struct sockaddr_storage spt_address;
> > >       __u16 spt_pathmaxrxt;
> > >       __u16 spt_pathpfthld;
> > > +     __u16 spt_pathcpthld;
> > >  };
> > >
> > >  /*
> > > diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> > > index 5e2098b..5b9774d 100644
> > > --- a/net/sctp/socket.c
> > > +++ b/net/sctp/socket.c
> > > @@ -3954,6 +3954,9 @@ static int sctp_setsockopt_paddr_thresholds(struct sock *sk,
> >
> > This code does:
> >         if (optlen < sizeof(struct sctp_paddrthlds))
> >                 return -EINVAL;
> here will become:
> 
>         if (optlen >= sizeof(struct sctp_paddrthlds)) {
>                 optlen = sizeof(struct sctp_paddrthlds);
>         } else if (optlen >= ALIGN(offsetof(struct sctp_paddrthlds,
>                                             spt_pathcpthld), 4))
>                 optlen = ALIGN(offsetof(struct sctp_paddrthlds,
>                                         spt_pathcpthld), 4);
>                 val.spt_pathcpthld = 0xffff;
>         else {
>                 return -EINVAL;
>         }

Hmmm...
If the kernel has to default 'val.spt_pathcpthld = 0xffff'
then recompiling an existing application with the new uapi
header is going to lead to very unexpected behaviour.

The best you can hope for is that the application memset the
structure to zero.
But more likely it is 'random' on-stack data.

	David

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