[bug report] libceph: ceph_pg_is_split() may get stuck in infinite loop

Raphael Zimmer <[email protected]>
Newsgroups org.kernel.vger.ceph-devel
Message-ID <[email protected]>
Hi,
I noticed some problems in ceph_pg_is_split() (net/ceph/osdmap.c) that
may result in an infinite loop for certain values of old_pg_num and
new_pg_num.
The issue arises if old_pg_num and new_pg_num have values greater than
or equal to 2^31. In this case, calc_bits_of(old_pg_num) calculates 32
and assigns this value to old_bits. The next statement:
	int old_mask = (1 << old_bits) - 1;
results in a left shift operation of 32 bits, which is undefined
behavior when performed in 32-bit width. This should be avoided in any
case. If only the last 5 bits of the shift count are evaluated (as done
on my machine), old_mask will be 0.
Inside the for loop, next_bit is calculated as
	int next_bit = n << (old_bits - 1);
which results in either 2^31 or 0 depending on whether n is odd or even.
In the next step, s is calculated as
	u32 s = next_bit | pgid->seed;
which results in either s = pgid->seed or s = pgid->seed + 2^31.
If old_pg_num > pgid->seed | 2^31, the following if statement always
evaluates to true and the loop continues with the next iteration.
This causes the loop to never terminate.
If old_pg_num <= pgid->seed | 2^31, the following if statement only
evaluates to true in case of s = pgid->seed. With s = pgid->seed + 2^31,
the remaining part of the loop's body will be executed. If new_pg_num is
greater than pgid->seed + 2^31 the
	if(s >= new_pg_num)
		break;
will never exit the loop.
Subsequently,
	s = ceph_stable_mod(s, old_pg_num, old_mask);
will return s = 0, and the return statement is only executed for
pgid->seed == 0. (In case the undefined shift operation evaluates the
same way as seen on my machine.)
If not, the loop never terminates.

This happens for all values that fulfill:
(new_pg_num > old_pg_num >= 2^31) and (old_pg_num > pgid->seed | 2^31)
|| ((new_pg_num > pgid->seed | 2^31) && pgid->seed > 0)
An actual example for this is new_pg_num == 2^32-1, old_pg_num == 2^32-2
and pgid->seed < 2^31-2.

Another case that results in a bit-shift being undefined behavior is
old_pg_num == 0. In this case, old_bits is 0, and the first statement
inside the for loop:
	int next_bit = n << (old_bits - 1);
results in n << -1, which is also undefined behavior.

In the first case, a misbehaving Ceph daemon or an attacker that
modifies messages can deliberately trigger infinite loops and thereby
force resource depletion.
This is the case because the values are read from the previous and
current osdmap, which have been received over the network and can
therefore contain arbitrary attacker-supplied data.

To handle these issues, additional logic could be introduced to
ceph_pg_is_split(). The infinite loop could be easily prevented by
adding an upper bound to the for loop. However, another solution would
be to not allow pg_num to take on such values directly when receiving an
osdmap. I'm not sure if these values can be treated as invalid. More
than 2^31 seems very unlikely to me, but 0 could possibly be a valid value.

Do you have an opinion or guidance on how to handle this, resp. which
possible solution to prefer? In this case, I can create a patch for it.

Best regards,
Raphael
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.