[Ocfs2-tools-devel] [RFC] PATCH: verify slot number in __ocfs2_read_slot_map(), v2

[Joel Becker]

On Thu, Mar 12, 2009 at 12:47:34AM +0800, Coly Li wrote:
> In some buggy conditions, mounted.ocfs2 does dirty reads, data from
> __ocfs2_read_slot_map() can not be trusted. When it happens, in ocfs2_print_nodes():
>  66                 node_num = map->md_slots[i].sd_node_num;
>  67                 if (names && names[node_num] && *(names[node_num]))
> node_num in 66 may be a very large number (due to the invalid data from
> __ocfs2_read_slot_map()), and names[node_num] references to an illegal memory
> region.

	The bug is in line 67, not in the slot map code.  See below for
comments on the actual patch.
	There are, in fact, multiple bugs here.  The real immediate bug
is that one should be checking that names is large enough to contaion
names[node_num].  You know, a length count for names.  That solves your
crash easily, and we're happy campers.
	However, there's a more subtle bug.   ocfs2_print_full_detect()
assumes the maximum node number is O2NM_MAX_NODES.  But that's the limit
for the o2cb cluster stack, not for the userspace stacks!
	It gets more fun; mounted.ocfs2(8) clearly only works for the
old stack, as it relies on o2cb_list_nodes(), and that doesn't work for
the userspace stack.
	Let's fix up the old stack first.  ocfs2_print_nodes() should
take a third argument, the length of the name list.  Thus you can check
if (node_num > length_of_names).  This fixes your crash, because you
pass 'i' from ocfs2_print_full_detect().
	Later we'll probably want to cook up a change that can get node
names from the userspace stacks.  But that will require more work.

	Ok, below here is commentary on why this patch doesn't work.
Don't worry about correcting these problems, just do a patch of
mounted.ocfs2(8) as described above.

> +/* es_node_num should be swapped to local cpu endian */
> +static errcode_t __ocfs2_verify_node_num(struct ocfs2_slot_map *sm,
> +					int num_slots)
> +{
> +	int i;
> +
> +	for (i = 0; i < num_slots; i++)
> +		if (sm->sm_slots[i] > OCFS2_MAX_SLOTS)
> +			return OCFS2_ET_INTERNAL_FAILURE;
> +	return 0;

	OCFS2_MAX_SLOTS is the maximum number of slots in the stot map,
not the maximum node number in a slot.  This is what Sunil means by the
comparison being invalid.  The only truly invalid node number in the old
slot map is (u16)-1 (65535).

> +static errcode_t __ocfs2_verify_node_num_extended(struct
> ocfs2_slot_map_extended *se,

	Btw, fix your email client.  It's wrapping these lines causing
corrupt patches.

> +						int num_slots)
> +{
> +	int i;
> +	for (i = 0; i < num_slots; i++)
> +		if (se->se_slots[i].es_node_num > OCFS2_MAX_SLOTS)
> +			return OCFS2_ET_INTERNAL_FAILURE;
> +	return 0;
> +}

	Here it's even worse.  The extended slot map supports 32bit node
numbers.  Any node number is valid, and our userspace cluster stacks
will definitely use numbers larger than 255!

Joel

-- 

Life's Little Instruction Book #207

	"Swing for the fence."

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker at oracle.com
Phone: (650) 506-8127