Re: [PATCH] vsock/virtio: fix kernel panic from virtio_transport_reset_no_sock

[Stefan Hajnoczi <stefanha@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 1475 bytes --]

On Tue, Mar 05, 2019 at 08:01:45PM +0200, Adalbert Lazăr wrote:

Thanks for the patch, Adalbert!  Please add a Signed-off-by tag so your
patch can be merged (see Documentation/process/submitting-patches.rst
Chapter 11 for details on the Developer's Certificate of Origin).

>  static int virtio_transport_reset_no_sock(struct virtio_vsock_pkt *pkt)
>  {
> +	const struct virtio_transport *t;
>  	struct virtio_vsock_pkt_info info = {
>  		.op = VIRTIO_VSOCK_OP_RST,
>  		.type = le16_to_cpu(pkt->hdr.type),
> @@ -680,7 +681,11 @@ static int virtio_transport_reset_no_sock(struct virtio_vsock_pkt *pkt)
>  	if (!pkt)
>  		return -ENOMEM;
>  
> -	return virtio_transport_get_ops()->send_pkt(pkt);
> +	t = virtio_transport_get_ops();
> +	if (!t)
> +		return -ENOTCONN;

pkt is leaked here.  This is an easy mistake to make because the code is
unclear.  The pkt argument is the received packet that we must reply to.
The reply packet is allocated just before line 680 and must be free
explicitly for return -ENOTCONN.

You can avoid the leak and make the code easier to read like this:

  struct virtio_vsock_pkt *reply;

  ...

     ------ avoid reusing 'pkt'
    v
  reply = virtio_transport_alloc_pkt(&info, 0, ...);
  if (!reply)
      return -ENOMEM;

  t = virtio_transport_get_ops();
  if (!t) {
      virtio_transport_free_pkt(reply); <-- prevent memory leak
      return -ENOTCONN;
  }
  return t->send_pkt(reply);

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]