[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