Re: [PATCH 7/8] xen/netfront: don't trust the backend response data blindly

[Jan Beulich <jbeulich@xxxxxxxx>]

On 13.05.2021 12:03, Juergen Gross wrote:
> @@ -429,6 +453,12 @@ static void xennet_tx_buf_gc(struct netfront_queue 
> *queue)
>       } while (more_to_do);
>  
>       xennet_maybe_wake_tx(queue);
> +
> +     return;
> +
> + err:
> +     queue->info->broken = true;
> +     dev_alert(dev, "Disabled for further use\n");
>  }

If in blkfront the ability to revive a device via a suspend/resume cycle
is "a nice side effect", wouldn't it be nice for all frontends to behave
similarly in this regard? I.e. wouldn't you want to also clear this flag
somewhere? And shouldn't additionally / more generally a disconnect /
connect cycle allow proper operation again?

> @@ -472,6 +502,13 @@ static void xennet_tx_setup_grant(unsigned long gfn, 
> unsigned int offset,
>  
>       *tx = info->tx_local;
>  
> +     /*
> +      * The request is not in its final form, as size and flags might be
> +      * modified later, but even if a malicious backend will send a response
> +      * now, nothing bad regarding security could happen.
> +      */
> +     queue->tx_pending[id] = true;

I'm not sure I can agree with what the comment says. If the backend
sent a response prematurely, wouldn't the underlying slot(s) become
available for re-use, and hence potentially get filled / updated by
two parties?

Jan