netdev - Re: two bogus patches arising from CVE-2019-12381

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20200401124633.GH23604@breakpoint.cc>
Date:   Wed, 1 Apr 2020 14:46:33 +0200
From:   Florian Westphal <fw@...len.de>
To:     Charles Bryant <ch.4g7vxy-nbkl8p@...h.co.uk>
Cc:     netdev@...r.kernel.org
Subject: Re: two bogus patches arising from CVE-2019-12381

Charles Bryant <ch.4g7vxy-nbkl8p@...h.co.uk> wrote:
> I believe two patches from last year are mistaken. They are:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/commit/?id=95baa60a0da80a0143e3ddd4d3725758b4513825
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/commit/?id=425aa0e1d01513437668fa3d4a971168bbaa8515
> 
> Both of these make a function return immediately with ENOMEM if a kalloc()
> fails.  However in each case the function already correctly handled
> allocation failure later on. Furthermore, by making them exit early
> on allocation failure, it (very slightly) makes them worse as in some
> cases they might have correctly returned EADDRINUSE and not needed the
> allocated memory.
> 
> I think, therefore, that these changes should be reverted.

Both fixes are useless, as you explained above.

But they do not matter.  When GFP_KERNEL allocations fail the entire
system is screwed anyway.

So instead of revert, I would suggest that you wait until net-next
reopens, then:

For the first commit, send a patch that reverts, but also add a comment
that explains the error is handled below the loop.

For the second commit, send a patch that moves the allocation to where its
needed -- the spinlock was converted to a mutex so there is no need
for this ahead-of-time allocation anymore.