netdev - Re: [PATCH] Change judgment len position

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, 24 Oct 2018 22:48:52 +0200
From:   Willy Tarreau <w@....eu>
To:     Joe Perches <joe@...ches.com>
Cc:     David Miller <davem@...emloft.net>, wanghaifine@...il.com,
        netdev@...r.kernel.org, LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] Change judgment len position

On Wed, Oct 24, 2018 at 11:28:11AM -0700, Joe Perches wrote:
> On Wed, 2018-10-24 at 10:10 -0700, David Miller wrote:
> > From: Wang Hai <wanghaifine@...il.com>
> > Date: Wed, 24 Oct 2018 23:47:29 +0800
> > 
> > > To determine whether len is less than zero, it should be put before 
> > > the function min_t, because the return value of min_t is not likely 
> > > to be less than zero.
> []
> > > diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> []
> > > @@ -3301,11 +3301,11 @@ static int do_tcp_getsockopt(struct sock *sk, int level,
> > >       struct net *net = sock_net(sk);
> > >       int val, len;
> > >  
> > > +     len = min_t(unsigned int, len, sizeof(int));
> > > +
> > >       if (get_user(len, optlen))
> > >               return -EFAULT;
> > 
> > You can't be serious?
> 
> I'm not personally taken aback by this but
> there is the new Code of
> Conduct to consider.
> 
> John McEnroe earned quite a bit of his
> reputation as an 'enfant terrible' via a
> similar statement.
> 
> https://www.youtube.com/watch?v=t0hK1wyrrAU
> 
> Perhaps a different word choice next time in
> reply to submitters of ill-considered and/or
> defective patches could be useful.

Maybe but on this one I think we're really out of the scope of the CoC.

When you read this patch from an apparent first-time contributor (no
trace in either LKML, netdev or even google), the level of assurance
in the commit message is pretty good, showing that he's not at all a
beginner, which doesn't match at all the type of error seen in the
code, which doesn't even need to be compiled to see that it will emit
a warning and not work as advertised. Moreover, the commit message is
vague enough to seem it tries to cover the patch, and doesn't even
match what's done in the patch.

When you factor in the fact that the code opens a big but very discrete
vulnerability, I tend to think that in fact we're not facing a newbie
at all but someone deliberately trying to inject a subtle backdoor into
the kernel and disguise it as a vague bug fix, possibly even hoping that
it would find its way to -stable. I would not be surprised if this e-mail
address is a throw-away anonymous address created just for this occasion.

I could totally be wrong of course, but the clues are quite heavy here
as I find it hard to argue for a series of beginner's mistakes. If this
person really exists and can explain how we ended up there, I will of
course happily retract my suspicion and apologize.

Willy