linux-kernel - Re: [PATCH net v2 0/5] Make TCP-MD5-diag slightly less broken

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: <CAJwJo6ZTT28XZ1HFhC77KrPeFmwVWDkFzYsg7YU1MD0PESAWrw@mail.gmail.com>
Date: Wed, 20 Nov 2024 16:13:42 +0000
From: Dmitry Safonov <0x7f454c46@...il.com>
To: Johannes Berg <johannes@...solutions.net>
Cc: Jakub Kicinski <kuba@...nel.org>, 
	Dmitry Safonov via B4 Relay <devnull+0x7f454c46.gmail.com@...nel.org>, 
	"David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, 
	Paolo Abeni <pabeni@...hat.com>, Simon Horman <horms@...nel.org>, David Ahern <dsahern@...nel.org>, 
	Ivan Delalande <colona@...sta.com>, Matthieu Baerts <matttbe@...nel.org>, 
	Mat Martineau <martineau@...nel.org>, Geliang Tang <geliang@...nel.org>, 
	John Fastabend <john.fastabend@...il.com>, Davide Caratti <dcaratti@...hat.com>, 
	Kuniyuki Iwashima <kuniyu@...zon.com>, netdev@...r.kernel.org, linux-kernel@...r.kernel.org, 
	mptcp@...ts.linux.dev
Subject: Re: [PATCH net v2 0/5] Make TCP-MD5-diag slightly less broken

On Wed, 20 Nov 2024 at 08:44, Johannes Berg <johannes@...solutions.net> wrote:
>
> (Sorry, late to the party)

Thanks for joining! :-)

> On Fri, 2024-11-15 at 16:08 -0800, Jakub Kicinski wrote:
> > Would it be too ugly if we simply retried with a 32kB skb if the initial
> > dump failed with EMSGSIZE?
>
> We have min_dump_alloc, which a number of places are setting much higher
> than the default, partially at least because there were similar issues,
> e.g. in nl80211. See e.g. nl80211_dump_wiphy() doing it dynamically.

Yeah, your example seems alike what netlink_dump() does with
min_dump_alloc and max_recvmsg_len. You have there
.doit = nl80211_get_wiphy,
.dumpit = nl80211_dump_wiphy,

So at this initial patch set, I'm trying to fix
inet_diag_handler::dump_one() callback, which is to my understanding
same as .doit() for generic netlink [should we just rename struct
inet_diag_handler callbacks to match the generics?]. See
inet_diag_handler_cmd() and NLM_F_DUMP in
Documentation/userspace-api/netlink/intro.rst
For TCP-MD5-diag even the single message reply may have a variable
number of keys on a socket's dump. For multi-messages dump, my plan is
to use netlink_callback::ctx[] and add an iterator type that will
allow to stop on N-th key between recvmsg() calls.

> Kind of ugly? Sure! And we shouldn't use it now with newer userspace
> that knows to request a more finely split dump. For older userspace it's
> the only way though.

Heh, the comment in nl80211_dump_wiphy() on sending an empty message
and retrying is ouch!

>
> Also, we don't even give all the data to older userspace (it must
> support split dumps to get information about the more modern features, 6
> GHz channels, etc.), but I gather that's not an option here.
>
> > Another option would be to automatically grow the skb. The size
> > accounting is an endless source of bugs. We'd just need to scan
> > the codebase to make sure there are no cases where someone does
> >
> >       ptr = __nla_reserve();
> >       nla_put();
> >       *ptr = 0;
> >
> > Which may be too much of a project and source of bugs in itself.
> >
> > Or do both, retry as a fix, and auto-grow in net-next.
>
> For auto-grow you'd also have to have information about the userspace
> buffer, I think? It still has to fit there, might as well fail anyway if
> that buffer is too small? I'm not sure we have that link back? But I'm
> not really sure right now, just remember this as an additional wrinkle
> from the above-mentioned nl80211 problem.

Yeah, netlink_recvmsg() attempts to track what the userspace is asking:

:        /* Record the max length of recvmsg() calls for future allocations */
:        max_recvmsg_len = max(READ_ONCE(nlk->max_recvmsg_len), len);
:        max_recvmsg_len = min_t(size_t, max_recvmsg_len,
:                                SKB_WITH_OVERHEAD(32768));
:        WRITE_ONCE(nlk->max_recvmsg_len, max_recvmsg_len);

Thanks,
             Dmitry