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: <20200523071929.GA10466@lst.de>
Date:   Sat, 23 May 2020 09:19:29 +0200
From:   'Christoph Hellwig' <hch@....de>
To:     Marcelo Ricardo Leitner <marcelo.leitner@...il.com>
Cc:     David Laight <David.Laight@...lab.com>,
        '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

On Fri, May 22, 2020 at 11:36:23AM -0300, Marcelo Ricardo Leitner wrote:
> 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.
> 
> For example, I can spot things like this easier (from
> [PATCH 26/49] sctp: pass a kernel pointer to sctp_setsockopt_auth_key)
> 
> @@ -3646,7 +3641,6 @@ static int sctp_setsockopt_auth_key(struct sock *sk,
>         }
> 
>  out:
> -       kzfree(authkey);
>         return ret;
>  }
> ...
> @@ -4771,7 +4765,10 @@ static int sctp_setsockopt(struct sock *sk, int level, int optname,
>         }
> 
>         release_sock(sk);
> -       kfree(kopt);
> +       if (optname == SCTP_AUTH_KEY)
> +               kzfree(kopt);
> +       else
> +               kfree(kopt);
> 
>  out_nounlock:
>         return retval;
> 
> these are 1k lines apart.
> 
> Yet, your implementation around this is better:
> 
> @@ -3733,7 +3624,7 @@ static int sctp_setsockopt_auth_key(struct sock *sk,
>         }
> 
>  out:
> -       kzfree(authkey);
> +       memset(authkey, 0, optlen);
>         return ret;
>  }
> 
> so that sctp_setsockopt() doesn't have to handle it specially.

Actually that implementation is wrong, if you want to move to a plain
kfree it would have to be a memzero_explicit.

> 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:

I don't really want to waste too much time on this, as what I really
need is to get the kernel_setsockopt removal series in ASAP.  I'm happy
to respin this once or twice with clear maintainer guidance (like the
memzero_explicit), but I have no idea what you even meant with your
other example or naming.  Tell me what exact changes you want, and
I can do a quick spin, but I don't really want a huge open ended
discussion on how to paint the bikeshed..

Alternatively I'll also happily only do a partial conversion for what
I need for the kernel_setsockopt removal and let you and Dave decided
what you guys prefer for the rest.