netdev - Re: [PATCH net 0/4] various compat ioctl fixes

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: <149d1ddec433d7cb766c99eeb78b220b33090287.camel@sipsolutions.net>
Date:   Mon, 28 Jan 2019 22:32:30 +0100
From:   Johannes Berg <johannes@...solutions.net>
To:     David Miller <davem@...emloft.net>
Cc:     netdev@...r.kernel.org, viro@...iv.linux.org.uk,
        robert@...llahan.org
Subject: Re: [PATCH net 0/4] various compat ioctl fixes

On Mon, 2019-01-28 at 11:22 -0800, David Miller wrote:

> I see some back and forth between you and Al, where do we stand at
> this point?

I don't really know. I think neither of us _likes_ this code, in
particular the whole copy_in_user() thing is quite a mess. The
copy_in_user() also means that decnet (and similar things, if they
exist, I didn't see any but didn't audit all protocols carefully) have
no way of working in compat - it's not even clear to me if that'd return
-EFAULT or just do something really stupid, and maybe even dangerous?

(Dangerous because at least on x86, compat_alloc_user_space() uses stack
space, and if we alloc 40 bytes but decnet writes up to 42 (?) then we
could overwrite some stack by that? Maybe the 16-byte alignment in
compat_alloc_user_space() saves us, but it's all very fragile. Even with
the previous patch fixed, decnet's idea of "struct ifreq" is bigger than
"struct ifreq" actually is because sockaddr_dn is bigger, if I'm
counting it right then that's 42 in total)

At the same time, fixing all this _completely_ is not very realistic, it
would require passing the ifreq size through to lots of places and
making the user copy there take the size rather than sizeof(ifreq),
obviously the very least to the method decnet uses, i.e. sock->ioctl() I
think, but clearly that affects every other protocol too.
This was what my previous patch had done partially for the directly
handled ioctls (the revert of which is the first patch in this series).

> From what I can see this looks like probably the simplest way to
> fix this in net and -stable currently.

I tend to agree, at least to fix the regression.

We can still deliberate separately if we want to fix decnet for compat
or if nobody cares now. But perhaps better decnet broken (quite
obviously and detectably) like it basically always was, than IP broken
(subtly, if your struct ends up landing at the end of a page).

Al, care to speak up about this here?

johannes