| Message ID | 20190125214320.17685-1-johannes@sipsolutions.net |
|---|---|
| Headers | show |
| Series | various compat ioctl fixes | expand |
From: Johannes Berg <johannes@sipsolutions.net> Date: Fri, 25 Jan 2019 22:43:16 +0100 > Back a long time ago, I already fixed a few of these by passing > the size of the struct ifreq to do_sock_ioctl(). However, Robert > found more cases, and now it won't be as simple because we'd have > to pass that down all the way to e.g. bond_do_ioctl() which isn't > really feasible. > > Therefore, restore the old code. > > While looking at why SIOCGIFNAME was broken, I realized that Al > had removed that case - which had been handled in an explicit > separate function - as well, and looking through his work at the > time I saw that bond ioctls were also affected by the erroneous > removal. > > I've restored SIOCGIFNAME and bond ioctls by going through the > (now renamed) dev_ifsioc() instead of reintroducing their own > helper functions, which I hope is correct but have only tested > with SIOCGIFNAME. I see some back and forth between you and Al, where do we stand at this point? From what I can see this looks like probably the simplest way to fix this in net and -stable currently. Please let me know. Thanks.
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
From: Johannes Berg <johannes@sipsolutions.net> Date: Mon, 28 Jan 2019 22:32:30 +0100 > Al, care to speak up about this here? I'll give Al one day to respond. I'll apply this series if he agrees or fails to give feedback.
On Mon, Jan 28, 2019 at 10:32:30PM +0100, Johannes Berg wrote: > 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? Umm... Short-term I don't see anything better; long-term I would really like to see compat_alloc_user_space()/copy_in_user() crap gone and copyin-copyout for anything more or less generic lifted up as far as cleanly possible, but let's not mix it with regression fixing. So for the lack of better short-term solutions, Acked-by: Al Viro <viro@zeniv.linux.org.uk> on the series.
From: Al Viro <viro@zeniv.linux.org.uk> Date: Wed, 30 Jan 2019 15:40:09 +0000 > On Mon, Jan 28, 2019 at 10:32:30PM +0100, Johannes Berg wrote: > >> 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? > > Umm... Short-term I don't see anything better; long-term I would really > like to see compat_alloc_user_space()/copy_in_user() crap gone and > copyin-copyout for anything more or less generic lifted up as far as > cleanly possible, but let's not mix it with regression fixing. It's a real shame, I thought it was a super clever solution to that problem space at the time we added it. > So for the lack of better short-term solutions, > Acked-by: Al Viro <viro@zeniv.linux.org.uk> > on the series. Ok, series applied, thanks everyone. I'll queue this up for -stable too.