Re: [PATCH 2/5] tap: Don't attempt to carry on if we get a bad frame length from qemu

[Stefano Brivio <sbrivio@redhat.com>]

On Fri, 26 Jul 2024 17:20:28 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> If we receive a too-short or too-long frame from the QEMU socket, currently
> we try to skip it and carry on.  That sounds sensible on first blush, but
> probably isn't wise in practice.  If this happens, either (a) qemu has done
> something seriously unexpected, or (b) we've received corrupt data over a
> Unix socket.  Or more likely (c), we have a bug elswhere which has put us
> out of sync with the stream, so we're trying to read something that's not a
> frame length as a frame length.
> 
> Neither (b) nor (c) is really salvageable with the same stream.  Case (a)
> might be ok, but we can no longer be confident qemu won't do something else
> we can't cope with.
> 
> So, instead of just skipping the frame and trying to carry on, log an error
> and close the socket.  As a bonus, establishing firm bounds on l2len early
> will allow simplifications to how we deal with the case where a partial
> frame is recv()ed.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  tap.c | 16 +++++++---------
>  1 file changed, 7 insertions(+), 9 deletions(-)
> 
> diff --git a/tap.c b/tap.c
> index a2ae7c2a..08700f65 100644
> --- a/tap.c
> +++ b/tap.c
> @@ -1013,7 +1013,13 @@ redo:
>  	}
>  
>  	while (n > (ssize_t)sizeof(uint32_t)) {
> -		ssize_t l2len = ntohl(*(uint32_t *)p);
> +		uint32_t l2len = ntohl(*(uint32_t *)p);
> +
> +		if (l2len < sizeof(struct ethhdr) || l2len > ETH_MAX_MTU) {
> +			err("Invalid frame size from QEMU (corrupt stream?)");

Same as my comment to 1/5, let me change this to:

			err("Bad frame size from guest, resetting connection");

-- 
Stefano