Message-ID: <202411031920.BEF6CEBCD@keescook>
Date: Sun, 3 Nov 2024 19:43:01 -0800
From: Kees Cook <kees@...nel.org>
To: Jakub Kicinski <kuba@...nel.org>
Cc: "Gustavo A. R. Silva" <gustavoars@...nel.org>,
	Andrew Lunn <andrew+netdev@...n.ch>,
	Johannes Berg <johannes@...solutions.net>,
	David Ahern <dsahern@...nel.org>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>,
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-hardening@...r.kernel.org, Simon Horman <horms@...nel.org>
Subject: Re: [PATCH v2 1/4][next] uapi: socket: Introduce struct
 sockaddr_legacy

On Thu, Oct 31, 2024 at 06:01:45PM -0700, Jakub Kicinski wrote:
> On Thu, 24 Oct 2024 15:11:24 -0600 Gustavo A. R. Silva wrote:
> > + * This is the legacy form of `struct sockaddr`. The original `struct sockaddr`
> > + * was modified in commit b5f0de6df6dce ("net: dev: Convert sa_data to flexible
> > + * array in struct sockaddr") due to the fact that "One of the worst offenders
> > + * of "fake flexible arrays" is struct sockaddr". This means that the original
> > + * `char sa_data[14]` behaved as a flexible array at runtime, so a proper
> > + * flexible-array member was introduced.
> 
> This isn't spelled out in the commit messages AFACT so let me ask..
> Why aren't we reverting b5f0de6df6dce, then?
> Feels like the best solution would be to have a separate type with
> the flex array to clearly annotate users who treat it as such.
> Is that not going to work?
> 
> My noob reading of b5f0de6df6dce is that it was a simpler workaround
> for the previous problem, avoided adding a new type (and the conversion
> churn). But now we are adding a type and another workaround on top.
> Sorry if I'm misunderstanding. No question that the struct is a mess,
> but I don't feel like this is helping the messiness...

This is a pretty reasonable position. I think sockaddr turns out to be a
really difficult problem, given its exposure to UAPI. Linux has been
trying to deal with it for a while (e.g. sockaddr_storage), and perhaps
b5f0de6df6dce wasn't the right solution. (It looked correct since it
looks like what we've done in many other tricky places, but perhaps
sockaddr should be dealt with differently yet.)

So, the rationale with b5f0de6df6dce was to change things as minimally
as possible. The goal is to stop lying to the compiler about object
sizes, with the big lie being "this object ends with a 14 byte array".

I think this results in two primary goals:
1- make sockaddr-like objects unambiguously sized
2- do not break userspace

For 2, we cannot change the existing (and externally defined) struct
sockaddr. It will stay the historical horrible thing that it is.

The specific UAPI sockaddr-related things we have that are _fixed_ sizes
are all fine. These can be seen with:
	git grep '^struct sockaddr_' -- include/uapi/

For 1, we cannot use (the externally defined) sockaddr as-is since we
run into size problems. (Basically anything casting from sockaddr,
anything copying out of sockaddr, etc.)

The storage problem for sockaddr has been attempted to be addressed with
the internally defined sockaddr_storage, but this doesn't capture the
"I don't know how big this object is yet" issues associated with taking
a buffer of arbitrary size from userspace that is initially defined
as sockaddr.

If we internally add struct sockaddr_unknown, or _bytes, or _unspec, or
_flex, or _array, or _raw, we can use that everywhere in the kernel, but
it is going to cause a lot of churn. This is why b5f0de6df6dce happened,
and why we went the direction of thinking sockaddr_legacy would be
workable: the kernel's view of sockaddr from userspace (sockaddr_legacy)
would be the only change needed.

Now, if we want to get to a place with the least ambiguity, we need to
abolish sockaddr from the kernel internally, and I think that might take
a while. I only think so because of what I went through just for doing a
portion of NFS in cf0d7e7f4520 ("NFS: Avoid memcpy() run-time warning
for struct sockaddr overflows").

But maybe it won't be as bad all that since we already do a lot of "what
is the family then cast to a fixed-size struct and carry on" stuff. But
the passing things between layers (usually "up" from a family into the
networking core) still depends on using sockaddr, and all of those APIs
would need to switch to sockaddr_unknown both in the core and in all
families. Maybe there is some Coccinelle magic that could be worked.

I think it's worth spending the time to try and figure out the size of
the effort needed to make that change.

-Kees

-- 
Kees Cook

Powered by blists - more mailing lists