On Wed, Jul 10, 2024 at 07:13:26PM +0200, Stefano Brivio wrote:On Wed, 10 Jul 2024 10:23:14 +1000 David Gibson <david(a)gibson.dropbear.id.au> wrote: > On Wed, Jul 10, 2024 at 12:32:33AM +0200, Stefano Brivio wrote: > > On Fri, 5 Jul 2024 12:07:18 +1000 > > David Gibson <david(a)gibson.dropbear.id.au> wrote:[snip]We can (see doc/platform-requirements/recv-zero.c). And even if we couldn't, we could use the same 1 byte buffer for all the datagrams. We basically just need the actual msghdr array.For UDP, we don't need to make the buffers large enough to fit packets into them, see udp(7): All receive operations return only one packet. When the packet is smaller than the passed buffer, only that much data is returned; when it is bigger, the packet is truncated and the MSG_TRUNC flag is set. so probably 1024 bytes (1 * UIO_MAXIOV) on the stack would be enough... or maybe we can even pass 0 as size and NULL buffers?Ah, interesting question.+ * need to. + */ + /* cppcheck-suppress nullPointer */ + while (recv(uflow->s[TGTSIDE], NULL, 0, MSG_DONTWAIT) >= 0) + ;Could a local attacker (another user) attempt to use this for denial of service?Of course, somebody could flood us anyway and we would get and handle all the events that that causes. But this case is different because we could get stuck for an unlimited amount of time without serving other sockets at all.Right.If that's a possibility, perhaps a limit for this loop (a maximum amount of recv()) tries would be a good idea. I'm not sure how we should handle the case where we exceed the threshold.We could fail to create the flow. That would limit the damage, but see below.Another one, which adds some complexity, but looks more correct to me, would be to try a single recv() call, and if we get data from it, fail creating the new flow entirely.Right. I also considered a single recvmmsg(); the difficulty with that is making suitable arrays for it, since the normal ones may be in use at this point.If recvmmsg() returns UIO_MAXIOV, then something weird is going on and we can abort the "connection" attempt.Seems reasonable; I've coded something up.Right. It feels like we're at least not making the guest dramatically more vulnerable to something here, so I'm inclined to treat it as good enough for now. -- David Gibson (he or they) | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you, not the other way | around. http://www.ozlabs.org/~dgibsonThis removes the potential DoS of wedging passt completely, but raises the possibility of a different one. Could an attacker - not necessarily on the same host, but network-closer than the guest's intended endpoint - prevent the guest from connecting to a particular service by spamming fake reply packets here.Oops, I didn't think about that, that's also concerning. With my suggestion above, 1024 packets is all an attacker would need. But maybe there's a safe way to deal with this that's still simpler than a separate handler: using recvmmsg(), going through msg_name of the (truncated) messages we received with it, and trying to draw some conclusion about what kind of attack we're dealing with from there. I'm not sure exactly which conclusion, though.I believe it would need to anticipate the guest's source port to do so, which might be good enough.Linux does source port randomisation for UDP starting from 2.6.24, so that should be good enough, but given that we try to preserve the source port from the guest, we might make a guest that doesn't do port randomisation (albeit unlikely to be found) vulnerable to this.