Commit 9a6b294a authored by David Howells's avatar David Howells Committed by Linus Torvalds
Browse files

afs: Fix use-after-free due to get/remove race in volume tree 



When an afs_volume struct is put, its refcount is reduced to 0 before
the cell->volume_lock is taken and the volume removed from the
cell->volumes tree.

Unfortunately, this means that the lookup code can race and see a volume
with a zero ref in the tree, resulting in a use-after-free:

    refcount_t: addition on 0; use-after-free.
    WARNING: CPU: 3 PID: 130782 at lib/refcount.c:25 refcount_warn_saturate+0x7a/0xda
    ...
    RIP: 0010:refcount_warn_saturate+0x7a/0xda
    ...
    Call Trace:
     afs_get_volume+0x3d/0x55
     afs_create_volume+0x126/0x1de
     afs_validate_fc+0xfe/0x130
     afs_get_tree+0x20/0x2e5
     vfs_get_tree+0x1d/0xc9
     do_new_mount+0x13b/0x22e
     do_mount+0x5d/0x8a
     __do_sys_mount+0x100/0x12a
     do_syscall_64+0x3a/0x94
     entry_SYSCALL_64_after_hwframe+0x62/0x6a

Fix this by:

 (1) When putting, use a flag to indicate if the volume has been removed
     from the tree and skip the rb_erase if it has.

 (2) When looking up, use a conditional ref increment and if it fails
     because the refcount is 0, replace the node in the tree and set the
     removal flag.

Fixes: 20325960 ("afs: Reorganise volume and server trees to be rooted on the cell")
Signed-off-by: default avatarDavid Howells <dhowells@redhat.com>
Reviewed-by: default avatarJeffrey Altman <jaltman@auristor.com>
cc: Marc Dionne <marc.dionne@auristor.com>
cc: linux-afs@lists.infradead.org
Signed-off-by: default avatarLinus Torvalds <torvalds@linux-foundation.org>
parent af73483f

fs/afs/internal.h

+2 −0
Original line number Diff line number Diff line
@@ -586,6 +586,7 @@ struct afs_volume { 
#define AFS_VOLUME_OFFLINE	4	/* - T if volume offline notice given */

#define AFS_VOLUME_BUSY		5	/* - T if volume busy notice given */

#define AFS_VOLUME_MAYBE_NO_IBULK 6	/* - T if some servers don't have InlineBulkStatus */

#define AFS_VOLUME_RM_TREE	7	/* - Set if volume removed from cell->volumes */

#ifdef CONFIG_AFS_FSCACHE

	struct fscache_volume	*cache;		/* Caching cookie */

#endif
@@ -1513,6 +1514,7 @@ extern struct afs_vlserver_list *afs_extract_vlserver_list(struct afs_cell *, 
extern struct afs_volume *afs_create_volume(struct afs_fs_context *);

extern int afs_activate_volume(struct afs_volume *);

extern void afs_deactivate_volume(struct afs_volume *);

bool afs_try_get_volume(struct afs_volume *volume, enum afs_volume_trace reason);

extern struct afs_volume *afs_get_volume(struct afs_volume *, enum afs_volume_trace);

extern void afs_put_volume(struct afs_net *, struct afs_volume *, enum afs_volume_trace);

extern int afs_check_volume_status(struct afs_volume *, struct afs_operation *);

fs/afs/volume.c

+23 −3
Original line number Diff line number Diff line
@@ -32,9 +32,14 @@ static struct afs_volume *afs_insert_volume_into_cell(struct afs_cell *cell, 
		} else if (p->vid > volume->vid) {

			pp = &(*pp)->rb_right;

		} else {

			volume = afs_get_volume(p, afs_volume_trace_get_cell_insert);

			if (afs_try_get_volume(p, afs_volume_trace_get_cell_insert)) {

				volume = p;

				goto found;

			}



			set_bit(AFS_VOLUME_RM_TREE, &volume->flags);

			rb_replace_node_rcu(&p->cell_node, &volume->cell_node, &cell->volumes);

		}

	}



	rb_link_node_rcu(&volume->cell_node, parent, pp);
@@ -56,6 +61,7 @@ static void afs_remove_volume_from_cell(struct afs_volume *volume) 
				 afs_volume_trace_remove);

		write_seqlock(&cell->volume_lock);

		hlist_del_rcu(&volume->proc_link);

		if (!test_and_set_bit(AFS_VOLUME_RM_TREE, &volume->flags))

			rb_erase(&volume->cell_node, &cell->volumes);

		write_sequnlock(&cell->volume_lock);

	}
@@ -231,6 +237,20 @@ static void afs_destroy_volume(struct afs_net *net, struct afs_volume *volume) 
	_leave(" [destroyed]");

}



/*

 * Try to get a reference on a volume record.

 */

bool afs_try_get_volume(struct afs_volume *volume, enum afs_volume_trace reason)

{

	int r;



	if (__refcount_inc_not_zero(&volume->ref, &r)) {

		trace_afs_volume(volume->vid, r + 1, reason);

		return true;

	}

	return false;

}



/*

 * Get a reference on a volume record.

 */