[PATCH v3 0/9] virtio: console: fixes for bugs and races with unplug

[PATCH v3 0/9] virtio: console: fixes for bugs and races with unplug

[Amit Shah]

Hello,

This series fixes a few bugs and races with port unplug and the
various file operations: read(), write() and close().

I started coding up an alternative locking mechanism based on the
discussion earlier in this series, but some of what we already have
has to remain, and the new code is sufficiently different, so I'd
rather it bakes for a while, and I ensure there are no regressions wrt
the tests I have so far for a while as well.  Hopefully this will be
in time for the next merge window.

There's one use-after-free I spotted after sending the first two
versions: port_fops_release() calls send_control_msg(), which spins
till the host acknowledges receipt of the buffer.  While it's
spinning, if the device gets unplugged, the vqs go away, and the
spinning function never progresses, causing a softlockup.  This is
difficult to reproduce -- the host usually acknowledges the buffers
fast enough.  A couple of solutions for this case are possible:

1. Mark the control vq in use, and don't proceed with unplug till it's
   marked unused,
2. Similar to the various port-specific i and o vqs, don't spin, but
   queue the buffer and wait for the host to let us know it's done
   with it.

2nd is easier to implement, but 1st fits with the way I'm thinking of
restructuring the locking.  I'm not yet decided on which approach to
take, will think over it.

Other than that, this series does fix all the bugs I see with the
tests I have.  Indeed, the patches marked for stable@ fix all the bugs
too, and the other ones on top add locking where shared structures are
being used.

Please review and apply if appropriate,

v3
 * remove patch 5, "update private_data in struct file only on
   successful open" (Rusty)
 * remove patch 6, "fix race in port_fops_poll() and port unplug",
   (Rusty)
 * remove CC: stable from patches without reproducers

v2
 * add patch 11: Jason found a use-after-free in port unplug
 * patch 7 introduced a regression where the wake_up_interruptible was
   done before guest_connected and host_connected were set to false

Amit Shah (9):
  virtio: console: fix race with port unplug and open/close
  virtio: console: fix race in port_fops_open() and port unplug
  virtio: console: clean up port data immediately at time of unplug
  virtio: console: fix raising SIGIO after port unplug
  virtio: console: return -ENODEV on all read operations after unplug
  virtio: console: add locks around buffer removal in port unplug path
  virtio: console: add locking in port unplug path
  virtio: console: fix locking around send_sigio_to_port()
  virtio: console: prevent use-after-free of port name in port unplug

 drivers/char/virtio_console.c | 64 +++++++++++++++++++++++++++----------------
 1 file changed, 40 insertions(+), 24 deletions(-)

-- 
1.8.1.4

[PATCH v3 1/9] virtio: console: fix race with port unplug and open/close

[Amit Shah]

There's a window between find_port_by_devt() returning a port and us
taking a kref on the port, where the port could get unplugged.  Fix it
by taking the reference in find_port_by_devt() itself.

Problem reported and analyzed by Mateusz Guzik.

CC: <stable@vger.kernel.org>
Reported-by: Mateusz Guzik <mguzik@redhat.com>
Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 drivers/char/virtio_console.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 1b456fe..291f437 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -272,9 +272,12 @@ static struct port *find_port_by_devt_in_portdev(struct ports_device *portdev,
 	unsigned long flags;
 
 	spin_lock_irqsave(&portdev->ports_lock, flags);
-	list_for_each_entry(port, &portdev->ports, list)
-		if (port->cdev->dev == dev)
+	list_for_each_entry(port, &portdev->ports, list) {
+		if (port->cdev->dev == dev) {
+			kref_get(&port->kref);
 			goto out;
+		}
+	}
 	port = NULL;
 out:
 	spin_unlock_irqrestore(&portdev->ports_lock, flags);
@@ -1019,14 +1022,10 @@ static int port_fops_open(struct inode *inode, struct file *filp)
 	struct port *port;
 	int ret;
 
+	/* We get the port with a kref here */
 	port = find_port_by_devt(cdev->dev);
 	filp->private_data = port;
 
-	/* Prevent against a port getting hot-unplugged at the same time */
-	spin_lock_irq(&port->portdev->ports_lock);
-	kref_get(&port->kref);
-	spin_unlock_irq(&port->portdev->ports_lock);
-
 	/*
 	 * Don't allow opening of console port devices -- that's done
 	 * via /dev/hvc
-- 
1.8.1.4

[PATCH v3 2/9] virtio: console: fix race in port_fops_open() and port unplug

[Amit Shah]

Between open() being called and processed, the port can be unplugged.
Check if this happened, and bail out.

A simple test script to reproduce this is:

while true; do for i in $(seq 1 100); do echo $i > /dev/vport0p3; done; done;

This opens and closes the port a lot of times; unplugging the port while
this is happening triggers the bug.

CC: <stable@vger.kernel.org>
Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 drivers/char/virtio_console.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 291f437..b04ec95 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1024,6 +1024,10 @@ static int port_fops_open(struct inode *inode, struct file *filp)
 
 	/* We get the port with a kref here */
 	port = find_port_by_devt(cdev->dev);
+	if (!port) {
+		/* Port was unplugged before we could proceed */
+		return -ENXIO;
+	}
 	filp->private_data = port;
 
 	/*
-- 
1.8.1.4

[PATCH v3 3/9] virtio: console: clean up port data immediately at time of unplug

[Amit Shah]

We used to keep the port's char device structs and the /sys entries
around till the last reference to the port was dropped.  This is
actually unnecessary, and resulted in buggy behaviour:

1. Open port in guest
2. Hot-unplug port
3. Hot-plug a port with the same 'name' property as the unplugged one

This resulted in hot-plug being unsuccessful, as a port with the same
name already exists (even though it was unplugged).

This behaviour resulted in a warning message like this one:

-------------------8<---------------------------------------
WARNING: at fs/sysfs/dir.c:512 sysfs_add_one+0xc9/0x130() (Not tainted)
Hardware name: KVM
sysfs: cannot create duplicate filename
'/devices/pci0000:00/0000:00:04.0/virtio0/virtio-ports/vport0p1'

Call Trace:
 [<ffffffff8106b607>] ? warn_slowpath_common+0x87/0xc0
 [<ffffffff8106b6f6>] ? warn_slowpath_fmt+0x46/0x50
 [<ffffffff811f2319>] ? sysfs_add_one+0xc9/0x130
 [<ffffffff811f23e8>] ? create_dir+0x68/0xb0
 [<ffffffff811f2469>] ? sysfs_create_dir+0x39/0x50
 [<ffffffff81273129>] ? kobject_add_internal+0xb9/0x260
 [<ffffffff812733d8>] ? kobject_add_varg+0x38/0x60
 [<ffffffff812734b4>] ? kobject_add+0x44/0x70
 [<ffffffff81349de4>] ? get_device_parent+0xf4/0x1d0
 [<ffffffff8134b389>] ? device_add+0xc9/0x650

-------------------8<---------------------------------------

Instead of relying on guest applications to release all references to
the ports, we should go ahead and unregister the port from all the core
layers.  Any open/read calls on the port will then just return errors,
and an unplug/plug operation on the host will succeed as expected.

This also caused buggy behaviour in case of the device removal (not just
a port): when the device was removed (which means all ports on that
device are removed automatically as well), the ports with active
users would clean up only when the last references were dropped -- and
it would be too late then to be referencing char device pointers,
resulting in oopses:

-------------------8<---------------------------------------
PID: 6162   TASK: ffff8801147ad500  CPU: 0   COMMAND: "cat"
 #0 [ffff88011b9d5a90] machine_kexec at ffffffff8103232b
 #1 [ffff88011b9d5af0] crash_kexec at ffffffff810b9322
 #2 [ffff88011b9d5bc0] oops_end at ffffffff814f4a50
 #3 [ffff88011b9d5bf0] die at ffffffff8100f26b
 #4 [ffff88011b9d5c20] do_general_protection at ffffffff814f45e2
 #5 [ffff88011b9d5c50] general_protection at ffffffff814f3db5
    [exception RIP: strlen+2]
    RIP: ffffffff81272ae2  RSP: ffff88011b9d5d00  RFLAGS: 00010246
    RAX: 0000000000000000  RBX: ffff880118901c18  RCX: 0000000000000000
    RDX: ffff88011799982c  RSI: 00000000000000d0  RDI: 3a303030302f3030
    RBP: ffff88011b9d5d38   R8: 0000000000000006   R9: ffffffffa0134500
    R10: 0000000000001000  R11: 0000000000001000  R12: ffff880117a1cc10
    R13: 00000000000000d0  R14: 0000000000000017  R15: ffffffff81aff700
    ORIG_RAX: ffffffffffffffff  CS: 0010  SS: 0018
 #6 [ffff88011b9d5d00] kobject_get_path at ffffffff8126dc5d
 #7 [ffff88011b9d5d40] kobject_uevent_env at ffffffff8126e551
 #8 [ffff88011b9d5dd0] kobject_uevent at ffffffff8126e9eb
 #9 [ffff88011b9d5de0] device_del at ffffffff813440c7

-------------------8<---------------------------------------

So clean up when we have all the context, and all that's left to do when
the references to the port have dropped is to free up the port struct
itself.

CC: <stable@vger.kernel.org>
Reported-by: chayang <chayang@redhat.com>
Reported-by: YOGANANTH SUBRAMANIAN <anantyog@in.ibm.com>
Reported-by: FuXiangChun <xfu@redhat.com>
Reported-by: Qunfang Zhang <qzhang@redhat.com>
Reported-by: Sibiao Luo <sluo@redhat.com>
Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 drivers/char/virtio_console.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index b04ec95..6bf0df3 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1501,14 +1501,6 @@ static void remove_port(struct kref *kref)
 
 	port = container_of(kref, struct port, kref);
 
-	sysfs_remove_group(&port->dev->kobj, &port_attribute_group);
-	device_destroy(pdrvdata.class, port->dev->devt);
-	cdev_del(port->cdev);
-
-	kfree(port->name);
-
-	debugfs_remove(port->debugfs_file);
-
 	kfree(port);
 }
 
@@ -1566,6 +1558,14 @@ static void unplug_port(struct port *port)
 	 */
 	port->portdev = NULL;
 
+	sysfs_remove_group(&port->dev->kobj, &port_attribute_group);
+	device_destroy(pdrvdata.class, port->dev->devt);
+	cdev_del(port->cdev);
+
+	kfree(port->name);
+
+	debugfs_remove(port->debugfs_file);
+
 	/*
 	 * Locks around here are not necessary - a port can't be
 	 * opened after we removed the port struct from ports_list
-- 
1.8.1.4

[PATCH v3 4/9] virtio: console: fix raising SIGIO after port unplug

[Amit Shah]

SIGIO should be sent when a port gets unplugged.  It should only be sent
to prcesses that have the port opened, and have asked for SIGIO to be
delivered.  We were clearing out guest_connected before calling
send_sigio_to_port(), resulting in a sigio not getting sent to
processes.

Fix by setting guest_connected to false after invoking the sigio
function.

CC: <stable@vger.kernel.org>
Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 drivers/char/virtio_console.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 6bf0df3..3435348 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1534,12 +1534,14 @@ static void unplug_port(struct port *port)
 	spin_unlock_irq(&port->portdev->ports_lock);
 
 	if (port->guest_connected) {
+		/* Let the app know the port is going down. */
+		send_sigio_to_port(port);
+
+		/* Do this after sigio is actually sent */
 		port->guest_connected = false;
 		port->host_connected = false;
-		wake_up_interruptible(&port->waitqueue);
 
-		/* Let the app know the port is going down. */
-		send_sigio_to_port(port);
+		wake_up_interruptible(&port->waitqueue);
 	}
 
 	if (is_console_port(port)) {
-- 
1.8.1.4

[PATCH v3 5/9] virtio: console: return -ENODEV on all read operations after unplug

[Amit Shah]

If a port gets unplugged while a user is blocked on read(), -ENODEV is
returned.  However, subsequent read()s returned 0, indicating there's no
host-side connection (but not indicating the device went away).

This also happened when a port was unplugged and the user didn't have
any blocking operation pending.  If the user didn't monitor the SIGIO
signal, they won't have a chance to find out if the port went away.

Fix by returning -ENODEV on all read()s after the port gets unplugged.
write() already behaves this way.

CC: <stable@vger.kernel.org>
Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 drivers/char/virtio_console.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 3435348..2b68075 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -749,6 +749,10 @@ static ssize_t port_fops_read(struct file *filp, char __user *ubuf,
 
 	port = filp->private_data;
 
+	/* Port is hot-unplugged. */
+	if (!port->guest_connected)
+		return -ENODEV;
+
 	if (!port_has_data(port)) {
 		/*
 		 * If nothing's connected on the host just return 0 in
@@ -765,7 +769,7 @@ static ssize_t port_fops_read(struct file *filp, char __user *ubuf,
 		if (ret < 0)
 			return ret;
 	}
-	/* Port got hot-unplugged. */
+	/* Port got hot-unplugged while we were waiting above. */
 	if (!port->guest_connected)
 		return -ENODEV;
 	/*
-- 
1.8.1.4

[PATCH v3 6/9] virtio: console: add locks around buffer removal in port unplug path

[Amit Shah]

The removal functions act on the vqs, and the vq operations need to be
locked.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 drivers/char/virtio_console.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 2b68075..9cbed93 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1512,18 +1512,22 @@ static void remove_port_data(struct port *port)
 {
 	struct port_buffer *buf;
 
+	spin_lock_irq(&port->inbuf_lock);
 	/* Remove unused data this port might have received. */
 	discard_port_data(port);
 
-	reclaim_consumed_buffers(port);
-
 	/* Remove buffers we queued up for the Host to send us data in. */
 	while ((buf = virtqueue_detach_unused_buf(port->in_vq)))
 		free_buf(buf, true);
+	spin_unlock_irq(&port->inbuf_lock);
+
+	spin_lock_irq(&port->outvq_lock);
+	reclaim_consumed_buffers(port);
 
 	/* Free pending buffers from the out-queue. */
 	while ((buf = virtqueue_detach_unused_buf(port->out_vq)))
 		free_buf(buf, true);
+	spin_unlock_irq(&port->outvq_lock);
 }
 
 /*
-- 
1.8.1.4

Re: [PATCH v3 6/9] virtio: console: add locks around buffer removal in port unplug path

[Rusty Russell]

Amit Shah <amit.shah@redhat.com> writes:
> The removal functions act on the vqs, and the vq operations need to be
> locked.
>
> Signed-off-by: Amit Shah <amit.shah@redhat.com>

Applied,
Rusty.

> ---
>  drivers/char/virtio_console.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> index 2b68075..9cbed93 100644
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
> @@ -1512,18 +1512,22 @@ static void remove_port_data(struct port *port)
>  {
>  	struct port_buffer *buf;
>  
> +	spin_lock_irq(&port->inbuf_lock);
>  	/* Remove unused data this port might have received. */
>  	discard_port_data(port);
>  
> -	reclaim_consumed_buffers(port);
> -
>  	/* Remove buffers we queued up for the Host to send us data in. */
>  	while ((buf = virtqueue_detach_unused_buf(port->in_vq)))
>  		free_buf(buf, true);
> +	spin_unlock_irq(&port->inbuf_lock);
> +
> +	spin_lock_irq(&port->outvq_lock);
> +	reclaim_consumed_buffers(port);
>  
>  	/* Free pending buffers from the out-queue. */
>  	while ((buf = virtqueue_detach_unused_buf(port->out_vq)))
>  		free_buf(buf, true);
> +	spin_unlock_irq(&port->outvq_lock);
>  }
>  
>  /*
> -- 
> 1.8.1.4

[PATCH v3 7/9] virtio: console: add locking in port unplug path

[Amit Shah]

Port unplug can race with close() in port_fops_release().
port_fops_release() already takes the necessary locks, ensure
unplug_port() does that too.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 drivers/char/virtio_console.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 9cbed93..e8b707d 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1541,6 +1541,7 @@ static void unplug_port(struct port *port)
 	list_del(&port->list);
 	spin_unlock_irq(&port->portdev->ports_lock);
 
+	spin_lock_irq(&port->inbuf_lock);
 	if (port->guest_connected) {
 		/* Let the app know the port is going down. */
 		send_sigio_to_port(port);
@@ -1551,6 +1552,7 @@ static void unplug_port(struct port *port)
 
 		wake_up_interruptible(&port->waitqueue);
 	}
+	spin_unlock_irq(&port->inbuf_lock);
 
 	if (is_console_port(port)) {
 		spin_lock_irq(&pdrvdata_lock);
-- 
1.8.1.4

Re: [PATCH v3 7/9] virtio: console: add locking in port unplug path

[Rusty Russell]

Amit Shah <amit.shah@redhat.com> writes:
> Port unplug can race with close() in port_fops_release().
> port_fops_release() already takes the necessary locks, ensure
> unplug_port() does that too.
>
> Signed-off-by: Amit Shah <amit.shah@redhat.com>

Applied,
Rusty.

> ---
>  drivers/char/virtio_console.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> index 9cbed93..e8b707d 100644
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
> @@ -1541,6 +1541,7 @@ static void unplug_port(struct port *port)
>  	list_del(&port->list);
>  	spin_unlock_irq(&port->portdev->ports_lock);
>  
> +	spin_lock_irq(&port->inbuf_lock);
>  	if (port->guest_connected) {
>  		/* Let the app know the port is going down. */
>  		send_sigio_to_port(port);
> @@ -1551,6 +1552,7 @@ static void unplug_port(struct port *port)
>  
>  		wake_up_interruptible(&port->waitqueue);
>  	}
> +	spin_unlock_irq(&port->inbuf_lock);
>  
>  	if (is_console_port(port)) {
>  		spin_lock_irq(&pdrvdata_lock);
> -- 
> 1.8.1.4

[PATCH v3 8/9] virtio: console: fix locking around send_sigio_to_port()

[Amit Shah]

send_sigio_to_port() checks the value of guest_connected, which we
always modify under the inbuf_lock; make sure invocations of
send_sigio_to_port() have take the inbuf_lock around the call.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 drivers/char/virtio_console.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index e8b707d..4e380c1 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1670,7 +1670,9 @@ static void handle_control_message(struct ports_device *portdev,
 		 * If the guest is connected, it'll be interested in
 		 * knowing the host connection state changed.
 		 */
+		spin_lock_irq(&port->inbuf_lock);
 		send_sigio_to_port(port);
+		spin_unlock_irq(&port->inbuf_lock);
 		break;
 	case VIRTIO_CONSOLE_PORT_NAME:
 		/*
@@ -1790,13 +1792,13 @@ static void in_intr(struct virtqueue *vq)
 	if (!port->guest_connected && !is_rproc_serial(port->portdev->vdev))
 		discard_port_data(port);
 
+	/* Send a SIGIO indicating new data in case the process asked for it */
+	send_sigio_to_port(port);
+
 	spin_unlock_irqrestore(&port->inbuf_lock, flags);
 
 	wake_up_interruptible(&port->waitqueue);
 
-	/* Send a SIGIO indicating new data in case the process asked for it */
-	send_sigio_to_port(port);
-
 	if (is_console_port(port) && hvc_poll(port->cons.hvc))
 		hvc_kick();
 }
-- 
1.8.1.4

Re: [PATCH v3 8/9] virtio: console: fix locking around send_sigio_to_port()

[Rusty Russell]

Amit Shah <amit.shah@redhat.com> writes:
> send_sigio_to_port() checks the value of guest_connected, which we
> always modify under the inbuf_lock; make sure invocations of
> send_sigio_to_port() have take the inbuf_lock around the call.
>
> Signed-off-by: Amit Shah <amit.shah@redhat.com>

Applied,
Rusty.

> ---
>  drivers/char/virtio_console.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> index e8b707d..4e380c1 100644
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
> @@ -1670,7 +1670,9 @@ static void handle_control_message(struct ports_device *portdev,
>  		 * If the guest is connected, it'll be interested in
>  		 * knowing the host connection state changed.
>  		 */
> +		spin_lock_irq(&port->inbuf_lock);
>  		send_sigio_to_port(port);
> +		spin_unlock_irq(&port->inbuf_lock);
>  		break;
>  	case VIRTIO_CONSOLE_PORT_NAME:
>  		/*
> @@ -1790,13 +1792,13 @@ static void in_intr(struct virtqueue *vq)
>  	if (!port->guest_connected && !is_rproc_serial(port->portdev->vdev))
>  		discard_port_data(port);
>  
> +	/* Send a SIGIO indicating new data in case the process asked for it */
> +	send_sigio_to_port(port);
> +
>  	spin_unlock_irqrestore(&port->inbuf_lock, flags);
>  
>  	wake_up_interruptible(&port->waitqueue);
>  
> -	/* Send a SIGIO indicating new data in case the process asked for it */
> -	send_sigio_to_port(port);
> -
>  	if (is_console_port(port) && hvc_poll(port->cons.hvc))
>  		hvc_kick();
>  }
> -- 
> 1.8.1.4

[PATCH v3 9/9] virtio: console: prevent use-after-free of port name in port unplug

[Amit Shah]

Remove the debugfs path before freeing port->name, to prevent a possible
use-after-free.

Reported-by: Jason Wang <jasowang@redhat.com>
Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 drivers/char/virtio_console.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 4e380c1..e910bec 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1574,9 +1574,8 @@ static void unplug_port(struct port *port)
 	device_destroy(pdrvdata.class, port->dev->devt);
 	cdev_del(port->cdev);
 
-	kfree(port->name);
-
 	debugfs_remove(port->debugfs_file);
+	kfree(port->name);
 
 	/*
 	 * Locks around here are not necessary - a port can't be
-- 
1.8.1.4

Re: [PATCH v3 9/9] virtio: console: prevent use-after-free of port name in port unplug

[Rusty Russell]

Amit Shah <amit.shah@redhat.com> writes:
> Remove the debugfs path before freeing port->name, to prevent a possible
> use-after-free.
>
> Reported-by: Jason Wang <jasowang@redhat.com>
> Signed-off-by: Amit Shah <amit.shah@redhat.com>

Applied,
Rusty.

> ---
>  drivers/char/virtio_console.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> index 4e380c1..e910bec 100644
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
> @@ -1574,9 +1574,8 @@ static void unplug_port(struct port *port)
>  	device_destroy(pdrvdata.class, port->dev->devt);
>  	cdev_del(port->cdev);
>  
> -	kfree(port->name);
> -
>  	debugfs_remove(port->debugfs_file);
> +	kfree(port->name);
>  
>  	/*
>  	 * Locks around here are not necessary - a port can't be
> -- 
> 1.8.1.4

Re: [PATCH v3 9/9] virtio: console: prevent use-after-free of port name in port unplug

[Amit Shah]

On (Mon) 29 Jul 2013 [14:26:09], Rusty Russell wrote:
> Amit Shah <amit.shah@redhat.com> writes:
> > Remove the debugfs path before freeing port->name, to prevent a possible
> > use-after-free.
> >
> > Reported-by: Jason Wang <jasowang@redhat.com>
> > Signed-off-by: Amit Shah <amit.shah@redhat.com>
> 
> Applied,
> Rusty.

Hey Rusty, I don't see this patch in your virtio-next branch:

https://git.kernel.org/cgit/linux/kernel/git/rusty/linux.git/log/?h=virtio-next

The others are present there.  Looks like this one got missed?

		Amit

Re: [PATCH v3 9/9] virtio: console: prevent use-after-free of port name in port unplug

[Rusty Russell]

Amit Shah <amit.shah@redhat.com> writes:
> On (Mon) 29 Jul 2013 [14:26:09], Rusty Russell wrote:
>> Amit Shah <amit.shah@redhat.com> writes:
>> > Remove the debugfs path before freeing port->name, to prevent a possible
>> > use-after-free.
>> >
>> > Reported-by: Jason Wang <jasowang@redhat.com>
>> > Signed-off-by: Amit Shah <amit.shah@redhat.com>
>> 
>> Applied,
>> Rusty.
>
> Hey Rusty, I don't see this patch in your virtio-next branch:
>
> https://git.kernel.org/cgit/linux/kernel/git/rusty/linux.git/log/?h=virtio-next
>
> The others are present there.  Looks like this one got missed?

Actually, it's in my pending-rebases branch, since it requireds things
from the fixes branch.  Once the fixes branch has gone to Linus (this
week) I will merge it into the virtio-next branch then apply this on
top.

It's generally considered bad form to merge into -next branches, but
it's allowed for cases like this.  But I want to make sure no changes to
fixes are required before I merge it, hence the delay for some exposure
in linux-next.

Cheers,
Rusty.

Re: [PATCH v3 9/9] virtio: console: prevent use-after-free of port name in port unplug

[Amit Shah]

On (Thu) 01 Aug 2013 [10:29:35], Rusty Russell wrote:
> Amit Shah <amit.shah@redhat.com> writes:
> > On (Mon) 29 Jul 2013 [14:26:09], Rusty Russell wrote:
> >> Amit Shah <amit.shah@redhat.com> writes:
> >> > Remove the debugfs path before freeing port->name, to prevent a possible
> >> > use-after-free.
> >> >
> >> > Reported-by: Jason Wang <jasowang@redhat.com>
> >> > Signed-off-by: Amit Shah <amit.shah@redhat.com>
> >> 
> >> Applied,
> >> Rusty.
> >
> > Hey Rusty, I don't see this patch in your virtio-next branch:
> >
> > https://git.kernel.org/cgit/linux/kernel/git/rusty/linux.git/log/?h=virtio-next
> >
> > The others are present there.  Looks like this one got missed?
> 
> Actually, it's in my pending-rebases branch, since it requireds things
> from the fixes branch.  Once the fixes branch has gone to Linus (this
> week) I will merge it into the virtio-next branch then apply this on
> top.

Ah, thanks!

> It's generally considered bad form to merge into -next branches, but
> it's allowed for cases like this.  But I want to make sure no changes to
> fixes are required before I merge it, hence the delay for some exposure
> in linux-next.

Got it, thanks for the explanation!

		Amit

Re: [PATCH v3 1/9] virtio: console: fix race with port unplug and open/close

[Rusty Russell]

Amit Shah <amit.shah@redhat.com> writes:
> There's a window between find_port_by_devt() returning a port and us
> taking a kref on the port, where the port could get unplugged.  Fix it
> by taking the reference in find_port_by_devt() itself.
>
> Problem reported and analyzed by Mateusz Guzik.

This fix is clearly correct, but what about the other find_port_by_*
functions?

Applied,
Rusty.

>
> CC: <stable@vger.kernel.org>
> Reported-by: Mateusz Guzik <mguzik@redhat.com>
> Signed-off-by: Amit Shah <amit.shah@redhat.com>
> ---
>  drivers/char/virtio_console.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> index 1b456fe..291f437 100644
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
> @@ -272,9 +272,12 @@ static struct port *find_port_by_devt_in_portdev(struct ports_device *portdev,
>  	unsigned long flags;
>  
>  	spin_lock_irqsave(&portdev->ports_lock, flags);
> -	list_for_each_entry(port, &portdev->ports, list)
> -		if (port->cdev->dev == dev)
> +	list_for_each_entry(port, &portdev->ports, list) {
> +		if (port->cdev->dev == dev) {
> +			kref_get(&port->kref);
>  			goto out;
> +		}
> +	}
>  	port = NULL;
>  out:
>  	spin_unlock_irqrestore(&portdev->ports_lock, flags);
> @@ -1019,14 +1022,10 @@ static int port_fops_open(struct inode *inode, struct file *filp)
>  	struct port *port;
>  	int ret;
>  
> +	/* We get the port with a kref here */
>  	port = find_port_by_devt(cdev->dev);
>  	filp->private_data = port;
>  
> -	/* Prevent against a port getting hot-unplugged at the same time */
> -	spin_lock_irq(&port->portdev->ports_lock);
> -	kref_get(&port->kref);
> -	spin_unlock_irq(&port->portdev->ports_lock);
> -
>  	/*
>  	 * Don't allow opening of console port devices -- that's done
>  	 * via /dev/hvc
> -- 
> 1.8.1.4

Re: [PATCH v3 1/9] virtio: console: fix race with port unplug and open/close

[Amit Shah]

On (Mon) 29 Jul 2013 [14:18:52], Rusty Russell wrote:
> Amit Shah <amit.shah@redhat.com> writes:
> > There's a window between find_port_by_devt() returning a port and us
> > taking a kref on the port, where the port could get unplugged.  Fix it
> > by taking the reference in find_port_by_devt() itself.
> >
> > Problem reported and analyzed by Mateusz Guzik.
> 
> This fix is clearly correct, but what about the other find_port_by_*
> functions?

They don't need a kref -- the kref is only to be bumped when:

1. Initialising / Plugging in the port (add_port)
2. Opening the port (this fix)

Both these cases are now covered.  As part of the locking rework, the
other find_port_by_* functions may be reworked to set some state, like
port_in_use, instead of abusing guest_connected today.

		Amit

Re: [PATCH v3 3/9] virtio: console: clean up port data immediately at time of unplug

[Rusty Russell]

Amit Shah <amit.shah@redhat.com> writes:
> We used to keep the port's char device structs and the /sys entries
> around till the last reference to the port was dropped.  This is
> actually unnecessary, and resulted in buggy behaviour:
>
> 1. Open port in guest
> 2. Hot-unplug port
> 3. Hot-plug a port with the same 'name' property as the unplugged one
>
> This resulted in hot-plug being unsuccessful, as a port with the same
> name already exists (even though it was unplugged).
>
> This behaviour resulted in a warning message like this one:
>
> -------------------8<---------------------------------------
> WARNING: at fs/sysfs/dir.c:512 sysfs_add_one+0xc9/0x130() (Not tainted)
> Hardware name: KVM
> sysfs: cannot create duplicate filename
> '/devices/pci0000:00/0000:00:04.0/virtio0/virtio-ports/vport0p1'
>
> Call Trace:
>  [<ffffffff8106b607>] ? warn_slowpath_common+0x87/0xc0
>  [<ffffffff8106b6f6>] ? warn_slowpath_fmt+0x46/0x50
>  [<ffffffff811f2319>] ? sysfs_add_one+0xc9/0x130
>  [<ffffffff811f23e8>] ? create_dir+0x68/0xb0
>  [<ffffffff811f2469>] ? sysfs_create_dir+0x39/0x50
>  [<ffffffff81273129>] ? kobject_add_internal+0xb9/0x260
>  [<ffffffff812733d8>] ? kobject_add_varg+0x38/0x60
>  [<ffffffff812734b4>] ? kobject_add+0x44/0x70
>  [<ffffffff81349de4>] ? get_device_parent+0xf4/0x1d0
>  [<ffffffff8134b389>] ? device_add+0xc9/0x650
>
> -------------------8<---------------------------------------
>
> Instead of relying on guest applications to release all references to
> the ports, we should go ahead and unregister the port from all the core
> layers.  Any open/read calls on the port will then just return errors,
> and an unplug/plug operation on the host will succeed as expected.
>
> This also caused buggy behaviour in case of the device removal (not just
> a port): when the device was removed (which means all ports on that
> device are removed automatically as well), the ports with active
> users would clean up only when the last references were dropped -- and
> it would be too late then to be referencing char device pointers,
> resulting in oopses:
>
> -------------------8<---------------------------------------
> PID: 6162   TASK: ffff8801147ad500  CPU: 0   COMMAND: "cat"
>  #0 [ffff88011b9d5a90] machine_kexec at ffffffff8103232b
>  #1 [ffff88011b9d5af0] crash_kexec at ffffffff810b9322
>  #2 [ffff88011b9d5bc0] oops_end at ffffffff814f4a50
>  #3 [ffff88011b9d5bf0] die at ffffffff8100f26b
>  #4 [ffff88011b9d5c20] do_general_protection at ffffffff814f45e2
>  #5 [ffff88011b9d5c50] general_protection at ffffffff814f3db5
>     [exception RIP: strlen+2]
>     RIP: ffffffff81272ae2  RSP: ffff88011b9d5d00  RFLAGS: 00010246
>     RAX: 0000000000000000  RBX: ffff880118901c18  RCX: 0000000000000000
>     RDX: ffff88011799982c  RSI: 00000000000000d0  RDI: 3a303030302f3030
>     RBP: ffff88011b9d5d38   R8: 0000000000000006   R9: ffffffffa0134500
>     R10: 0000000000001000  R11: 0000000000001000  R12: ffff880117a1cc10
>     R13: 00000000000000d0  R14: 0000000000000017  R15: ffffffff81aff700
>     ORIG_RAX: ffffffffffffffff  CS: 0010  SS: 0018
>  #6 [ffff88011b9d5d00] kobject_get_path at ffffffff8126dc5d
>  #7 [ffff88011b9d5d40] kobject_uevent_env at ffffffff8126e551
>  #8 [ffff88011b9d5dd0] kobject_uevent at ffffffff8126e9eb
>  #9 [ffff88011b9d5de0] device_del at ffffffff813440c7
>
> -------------------8<---------------------------------------
>
> So clean up when we have all the context, and all that's left to do when
> the references to the port have dropped is to free up the port struct
> itself.
>
> CC: <stable@vger.kernel.org>
> Reported-by: chayang <chayang@redhat.com>
> Reported-by: YOGANANTH SUBRAMANIAN <anantyog@in.ibm.com>
> Reported-by: FuXiangChun <xfu@redhat.com>
> Reported-by: Qunfang Zhang <qzhang@redhat.com>
> Reported-by: Sibiao Luo <sluo@redhat.com>
> Signed-off-by: Amit Shah <amit.shah@redhat.com>

Applied, thanks.

Cheers,
Rusty.

> ---
>  drivers/char/virtio_console.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> index b04ec95..6bf0df3 100644
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
> @@ -1501,14 +1501,6 @@ static void remove_port(struct kref *kref)
>  
>  	port = container_of(kref, struct port, kref);
>  
> -	sysfs_remove_group(&port->dev->kobj, &port_attribute_group);
> -	device_destroy(pdrvdata.class, port->dev->devt);
> -	cdev_del(port->cdev);
> -
> -	kfree(port->name);
> -
> -	debugfs_remove(port->debugfs_file);
> -
>  	kfree(port);
>  }
>  
> @@ -1566,6 +1558,14 @@ static void unplug_port(struct port *port)
>  	 */
>  	port->portdev = NULL;
>  
> +	sysfs_remove_group(&port->dev->kobj, &port_attribute_group);
> +	device_destroy(pdrvdata.class, port->dev->devt);
> +	cdev_del(port->cdev);
> +
> +	kfree(port->name);
> +
> +	debugfs_remove(port->debugfs_file);
> +
>  	/*
>  	 * Locks around here are not necessary - a port can't be
>  	 * opened after we removed the port struct from ports_list
> -- 
> 1.8.1.4

Re: [PATCH v3 2/9] virtio: console: fix race in port_fops_open() and port unplug

[Rusty Russell]

Amit Shah <amit.shah@redhat.com> writes:
> Between open() being called and processed, the port can be unplugged.
> Check if this happened, and bail out.
>
> A simple test script to reproduce this is:
>
> while true; do for i in $(seq 1 100); do echo $i > /dev/vport0p3; done; done;
>
> This opens and closes the port a lot of times; unplugging the port while
> this is happening triggers the bug.
>
> CC: <stable@vger.kernel.org>
> Signed-off-by: Amit Shah <amit.shah@redhat.com>
> ---
>  drivers/char/virtio_console.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> index 291f437..b04ec95 100644
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
> @@ -1024,6 +1024,10 @@ static int port_fops_open(struct inode *inode, struct file *filp)
>  
>  	/* We get the port with a kref here */
>  	port = find_port_by_devt(cdev->dev);
> +	if (!port) {
> +		/* Port was unplugged before we could proceed */
> +		return -ENXIO;
> +	}
>  	filp->private_data = port;
>  
>  	/*
> -- 
> 1.8.1.4

Applied.

Thanks,
Rusty.

Re: [PATCH v3 4/9] virtio: console: fix raising SIGIO after port unplug

[Rusty Russell]

Amit Shah <amit.shah@redhat.com> writes:
> SIGIO should be sent when a port gets unplugged.  It should only be sent
> to prcesses that have the port opened, and have asked for SIGIO to be
> delivered.  We were clearing out guest_connected before calling
> send_sigio_to_port(), resulting in a sigio not getting sent to
> processes.
>
> Fix by setting guest_connected to false after invoking the sigio
> function.
>
> CC: <stable@vger.kernel.org>
> Signed-off-by: Amit Shah <amit.shah@redhat.com>

Applied,
Rusty.

> ---
>  drivers/char/virtio_console.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> index 6bf0df3..3435348 100644
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
> @@ -1534,12 +1534,14 @@ static void unplug_port(struct port *port)
>  	spin_unlock_irq(&port->portdev->ports_lock);
>  
>  	if (port->guest_connected) {
> +		/* Let the app know the port is going down. */
> +		send_sigio_to_port(port);
> +
> +		/* Do this after sigio is actually sent */
>  		port->guest_connected = false;
>  		port->host_connected = false;
> -		wake_up_interruptible(&port->waitqueue);
>  
> -		/* Let the app know the port is going down. */
> -		send_sigio_to_port(port);
> +		wake_up_interruptible(&port->waitqueue);
>  	}
>  
>  	if (is_console_port(port)) {
> -- 
> 1.8.1.4

Re: [PATCH v3 5/9] virtio: console: return -ENODEV on all read operations after unplug

[Rusty Russell]

Amit Shah <amit.shah@redhat.com> writes:
> If a port gets unplugged while a user is blocked on read(), -ENODEV is
> returned.  However, subsequent read()s returned 0, indicating there's no
> host-side connection (but not indicating the device went away).
>
> This also happened when a port was unplugged and the user didn't have
> any blocking operation pending.  If the user didn't monitor the SIGIO
> signal, they won't have a chance to find out if the port went away.
>
> Fix by returning -ENODEV on all read()s after the port gets unplugged.
> write() already behaves this way.
>
> CC: <stable@vger.kernel.org>
> Signed-off-by: Amit Shah <amit.shah@redhat.com>
> ---
>  drivers/char/virtio_console.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)

Applied,
Rusty.

>
> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> index 3435348..2b68075 100644
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
> @@ -749,6 +749,10 @@ static ssize_t port_fops_read(struct file *filp, char __user *ubuf,
>  
>  	port = filp->private_data;
>  
> +	/* Port is hot-unplugged. */
> +	if (!port->guest_connected)
> +		return -ENODEV;
> +
>  	if (!port_has_data(port)) {
>  		/*
>  		 * If nothing's connected on the host just return 0 in
> @@ -765,7 +769,7 @@ static ssize_t port_fops_read(struct file *filp, char __user *ubuf,
>  		if (ret < 0)
>  			return ret;
>  	}
> -	/* Port got hot-unplugged. */
> +	/* Port got hot-unplugged while we were waiting above. */
>  	if (!port->guest_connected)
>  		return -ENODEV;
>  	/*
> -- 
> 1.8.1.4