From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail.ozlabs.org (mail.ozlabs.org [IPv6:2404:9400:2221:ea00::3]) by passt.top (Postfix) with ESMTPS id D2D5A5A004F for ; Thu, 27 Jun 2024 03:30:09 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202312; t=1719451805; bh=qSsSVt4ee8IUdUJMkXz+WZ0vKx5ekcHpidne9QqasKA=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=hZd2PauFYgmdbsXvmHt7QMAZgFrj3gL0j0JzH/Qmw5nk24SEr79g3d7RonWAGK5KR 0tac7K+kj/7U9zUsjRVDZC6XOS5EnCKWwqWUZ2zNnf1bX9DNZaic5rLmuHd7VrRiAN mOUbcC5EANicx9vNuhARoA2VgALFaPPlKmRuOZYTL8gcx65bXRKCR3xxD4vfvPTOHk OWVAeBjlTH890UXqQAfaN/hZGm1ebs8Y/DeQFHObQxv38mGeJ0dHBBY3votfgQ9Zwq 5FJmTR8z2Z0wBWC6ZY5abN0pEmPTGWajW5mZ/jNz3/5XpZWMrtz9GWrsHgLdyfsF0z 9rP3EdJWFMeuw== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4W8gws1YMsz4x7l; Thu, 27 Jun 2024 11:30:05 +1000 (AEST) Date: Thu, 27 Jun 2024 11:30:02 +1000 From: David Gibson To: Stefano Brivio Subject: Re: [PATCH 4/4] tap: Drop frames from guest whose length is more than remaining buffer Message-ID: References: <20240626234536.3306466-1-sbrivio@redhat.com> <20240626234536.3306466-5-sbrivio@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="/3an6Vf3YQjKLhAD" Content-Disposition: inline In-Reply-To: <20240626234536.3306466-5-sbrivio@redhat.com> Message-ID-Hash: WOKSQDIQZ2LUUTB7J4LN2ICCLQNYNHPT X-Message-ID-Hash: WOKSQDIQZ2LUUTB7J4LN2ICCLQNYNHPT X-MailFrom: dgibson@gandalf.ozlabs.org X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: passt-dev@passt.top, Matej Hrica X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: --/3an6Vf3YQjKLhAD Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Jun 27, 2024 at 01:45:36AM +0200, Stefano Brivio wrote: > This was reported by Matej a while ago, but we forgot to fix it. Even > if the hypervisor is necessarily trusted by passt, as it can in any > case terminate the guest or disrupt guest connectivity, it's a good > idea to be robust against possible issues. >=20 > Instead of resetting the connection to the hypervisor, just skip the > offending frame, as we had a few cases where QEMU would get the > length descriptor wrong, in the past. >=20 > Reported-by: Matej Hrica > Suggested-by: Matej Hrica > Signed-off-by: Stefano Brivio > --- > tap.c | 3 +++ > 1 file changed, 3 insertions(+) >=20 > diff --git a/tap.c b/tap.c > index 7f8c26d..bb993e0 100644 > --- a/tap.c > +++ b/tap.c > @@ -1026,6 +1026,9 @@ redo: So.. I just realised there's a different pre-existing problem here, above what's quoted in the patch we have: ssize_t l2len =3D ntohl(*(uint32_t *)p); On a platform where ssize_t is only 32-bits we could get a negative value here, which would be very bad. So l2len should be a uint32_t, not ssize_t. We do then need to make sure that the comparison between l2len and n is unsigned - it's safe to cast n to size_t there, because we've verified it's positive as the loop condition. Or... maybe it's simpler. The frame length is encoded as 32-bits, but we can't meaningfully have frames above 64k (maybe 64k+ETH_HLEN). So possibly we should just reset the tap connection if we see such a frame (most likely it means we've somehow gotten out of sync, anyway). > p +=3D sizeof(uint32_t); > n -=3D sizeof(uint32_t); > + if (l2len > (ssize_t)TAP_BUF_BYTES - n) I hate to discard valid frames from the guest. > + goto next; =2E.and this is not safe. This skips (l2len > n) check, which means that the n -=3D l2len at next could now have a signed overflow, which is UB. > + > /* At most one packet might not fit in a single read, and this > * needs to be blocking. > */ --=20 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/~dgibson --/3an6Vf3YQjKLhAD Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmZ8wJkACgkQzQJF27ox 2GcXYxAAqXnZggSIYUESv9Gbh8Arkr6ziG3mh89/Lo/XwYPEF+aWm6EvygadhI2p OQNqjrVb1ALQ8KLQJG8R/hrXfs39gI5E9a593JQlAogcuVQDqod6TjSZG7FPIlv+ SdS8LCC8XFavUtHliQXLpGRj/+aGM7Ueda/Br1AXPNuJsdufnwwiPGMBnPSU+ZEh DTeEBIHfzBdsVSmKzfOYVIme7htHjGozHB9MkJk7XVgf9ztFBm0Osx+s8X7DQ6qV NqMIwAvJRxTJ2iBXMU+kqNPAWZEg8ABi3L4anC9rlW91eVFiQhM8421MQ388NfOi 8MLIk5isJzYKNSkUvi8QYrrpHJONyvzFIr0VaBgPfmdNlWzg7lHyxPeMt93MEW5r 8+N2etMy0c2I0aeodQRz4jab1meTXxYUEbeVGvDjdZk+7rHk/OTU9siS33jTjZhk Jtt1YbFzTo9rBsr8ycG7b+7wU5K3fp9kn307oyta1g+6iNQF7Z2crNWm5tLqCPUN WKg9MeiqizAvbPKcOt03EfuMexUlDXaMwe6W0vg1YyzDOGJZ403vGfffTI6DIrqM R+mX9EXpsLXASwg5oN+rb/0NbBs3xS/L2IBjEvTzMMgIVL2gaXHUdlx3ijxNbdYF 6Z6egqP8bQ26/d9nLhUPDS1uvzLgvbM/77U3geyq1bwqbZlBoCk= =xaiR -----END PGP SIGNATURE----- --/3an6Vf3YQjKLhAD--