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

[Johannes Berg <johannes@sipsolutions.net>]

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