The virtio_fs object is refcounted via its kobject, so a mount can keep it
alive well beyond the lifetime of the virtio device binding. The tag
string, however, was allocated with devm_kmalloc() and is therefore freed
by devres_release_all() as soon as the device is unbound, i.e. right after
virtio_fs_remove() returns (see device_unbind_cleanup() in
drivers/base/dd.c).
virtio_fs_get_tree() looks up the instance, takes a kobject reference and
then drops virtio_fs_mutex before doing the sleepable part of the mount
(allocations, fuse_conn_init(), sget_fc()). A concurrent device removal
can complete in that window:
CPU 0 (mount) CPU 1 (remove)
------------- --------------
virtio_fs_get_tree()
virtio_fs_find_instance()
// reference taken, mutex dropped
virtio_fs_remove()
list_del_init(&fs->list);
// driver core unbinds device
// devres frees fs->tag
virtio_fs_fill_super()
mutex_lock(&virtio_fs_mutex);
if (list_empty(&fs->list))
pr_info(... fs->tag); <-- use-after-free
The virtio_fs object itself is still valid here, only its tag has been
freed behind its back.
Fix the lifetime mismatch at the source: allocate the tag with kmalloc()
and free it from virtio_fs_ktype_release(), so that it lives exactly as
long as the object embedding it. All probe error paths already drop the
final kobject reference, so they are covered. Merely dropping the tag
from the error message would paper over this instance while leaving a
devres pointer embedded in an object that routinely outlives its device.
The other readers of fs->tag are unaffected: tag_show() is fenced off by
the kobject_del() in virtio_fs_remove(), the automatic KOBJ_REMOVE uevent
is emitted from __kobject_del() while the device is still bound, and both
virtio_fs_add_instance() and virtio_fs_find_instance() only walk instances
still on virtio_fs_instances.
Fixes: a62a8ef9d97d ("virtio-fs: add virtiofs filesystem")
Cc: <[email protected]>
Reported-by: Sashiko <[email protected]>
Closes: https://sashiko.dev/#/bug/linux-0328b3a1-043b-468e-8425-6a613d91b5ef
Signed-off-by: Miklos Szeredi <[email protected]>
---
fs/fuse/virtio_fs.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
index ba8d79c4bcac..ef58bedc3941 100644
--- a/fs/fuse/virtio_fs.c
+++ b/fs/fuse/virtio_fs.c
@@ -193,6 +193,7 @@ static void virtio_fs_ktype_release(struct kobject *kobj)
{
struct virtio_fs *vfs = container_of(kobj, struct virtio_fs, kobj);
+ kfree(vfs->tag);
kfree(vfs->mq_map);
kfree(vfs->vqs);
kfree(vfs);
@@ -489,7 +490,11 @@ static void virtio_fs_free_devs(struct virtio_fs *fs)
}
}
-/* Read filesystem name from virtio config into fs->tag (must kfree()). */
+/*
+ * Read filesystem name from virtio config into fs->tag. Freed by
+ * virtio_fs_ktype_release() so that it stays valid for as long as the
+ * virtio_fs object itself, which may outlive the virtio device.
+ */
static int virtio_fs_read_tag(struct virtio_device *vdev, struct virtio_fs *fs)
{
char tag_buf[sizeof_field(struct virtio_fs_config, tag)];
@@ -505,7 +510,7 @@ static int virtio_fs_read_tag(struct virtio_device *vdev,
struct virtio_fs *fs)
end = &tag_buf[sizeof(tag_buf)];
len = end - tag_buf;
- fs->tag = devm_kmalloc(&vdev->dev, len + 1, GFP_KERNEL);
+ fs->tag = kmalloc(len + 1, GFP_KERNEL);
if (!fs->tag)
return -ENOMEM;
memcpy(fs->tag, tag_buf, len);
--
2.56.0
