netdev - RE: do a single memdup_user in 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]

Message-ID: <a599a1a6bed0412492bafdbeecf6bb4c@AcuMS.aculab.com>
Date:   Fri, 22 May 2020 15:52:43 +0000
From:   David Laight <David.Laight@...LAB.COM>
To:     'Marcelo Ricardo Leitner' <marcelo.leitner@...il.com>
CC:     'Christoph Hellwig' <hch@....de>,
        Vlad Yasevich <vyasevich@...il.com>,
        Neil Horman <nhorman@...driver.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        "linux-sctp@...r.kernel.org" <linux-sctp@...r.kernel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: RE: do a single memdup_user in sctp_setsockopt

From: Marcelo Ricardo Leitner
> Sent: 22 May 2020 15:36
> 
> On Fri, May 22, 2020 at 08:02:09AM +0000, David Laight wrote:
> > From: Christoph Hellwig
> > > Sent: 21 May 2020 18:47
> > > based on the review of Davids patch to do something similar I dusted off
> > > the series I had started a few days ago to move the memdup_user or
> > > copy_from_user from the inidividual sockopts into sctp_setsockopt,
> > > which is done with one patch per option, so it might suit Marcelo's
> > > taste a bit better.  I did not start any work on getsockopt.
> >
> > I'm not sure that 49 patches is actually any easier to review.
> > Most of the patches are just repetitions of the same change.
> > If they were in different files it might be different.
> 
> It's subjective, yes, but we hardly have patches over 5k lines.
> In the case here, as changing the functions also requires changing
> their call later on the file, it helps to be able to check that is was
> properly updated. Ditto for chained functions.

Between them sparse and the compiler rather force you to find everything.
The main danger was failing to change sizeof(param) to sizeof(*param)
and I double-checked all the relevant lines/

...
> What if you two work on a joint patchset for this? The proposals are
> quite close. The differences around the setsockopt handling are
> minimal already. It is basically variable naming, indentation and one
> or another small change like:

If the changes match then the subfunctions are probably fine.

Because I've got at least 64 bytes I can convert in-situ and assume
(in getsockopt()) that I can action the request (if it only only a read)
and check the length later.
With only a memdup_user() you can't make those changes.


> From Christoph's to David's:
> @@ -2249,11 +2248,11 @@ static int sctp_setsockopt_autoclose(struct sock *sk, u32 *autoclose,
>                 return -EOPNOTSUPP;
>         if (optlen != sizeof(int))
>                 return -EINVAL;
> -
> -       if (*autoclose > net->sctp.max_autoclose)
> +
> +       sp->autoclose = *optval;
> +
> +       if (sp->autoclose > net->sctp.max_autoclose)
>                 sp->autoclose = net->sctp.max_autoclose;
> -       else
> -               sp->autoclose = *autoclose;

I was trying not to make extra changes.
(Apart from error path ones.)
Clearly that should be:
	sp->autoclose = min(*optval, net->sctp.max_autoclose);
But that requires additional thought.

> > If you try to do getsockopt() the same way it will be much
> > more complicated - you have to know whether the called function
> > did the copy_to_user() and then suppress it.
> 
> If it is not possible, then the setsockopt one already splited half of
> the lines of the patch. :-)

Apart from the getsockopt() that is really a setsockopt() (CONNECTX3).
That might tie you in real knots.

	David

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