On Tue, Jul 10, 2018 at 12:27 AM, Rasmus Villemoes <linux@xxxxxxxxxxxxxxxxxx> wrote: > On 2018-07-09 19:57, Kees Cook wrote: >> On Sun, Jul 8, 2018 at 3:38 AM, Leon Romanovsky <leon@xxxxxxxxxx> >> wrote: >>> From: Leon Romanovsky <leonro@xxxxxxxxxxxx> >>> >>> Expand existing check_*_overflow tests to support >>> check_shift_overflow(). >>> >>> Signed-off-by: Leon Romanovsky <leonro@xxxxxxxxxxxx> >> >> This is a painful diff due to the new field > > Yeah, and that's not the right way to do these tests. Unlike +, - and *, > a and s do not play a symmetrical role in a<<s, so it doesn't make sense > to try to shoehorn the tests into those tables, not to mention that most > of the existing b values are obviously garbage when used as s. Moreover, > by the nature of those tables and the decision to force type hygiene for > the existing check_*_overflow, there are no tests where typeof(a) and > typeof(*d) have different signedness, which I specifically asked for. > > And, if we really don't want to define the result in case of some kind > of overflow (which I don't like, but OTOH agree that it's hard to define > sensible semantics), the test cases should not enshrine some specific > result - whatever the implementation happens to produce currently - that > makes it harder to change the implementation. This also makes them unfit > for the existing scheme. > > In short, I strongly disagree with the cover letter's "but good enough". > > So please, do test the mixed-types and in particular mixed-signedness, > and the corner cases of "the shift is all right, but then we do a > narrowing store", or "a is 0, but still catch bogus shift counts". > Something like this with lots of more test cases (and sorry if this > turns out whitespace damaged) I'm trying to beef up the regression tests here, based on your example. I wanted to check one assumption to see if I was off in a useless corner or not. Shifting into the signed bit is correctly detected: test_overflow: ok: (s8)(1 << 7) == overflow test_overflow: ok: (s16)(1 << 15) == overflow test_overflow: ok: (int)(1 << 31) == overflow test_overflow: ok: (s32)(1 << 31) == overflow test_overflow: ok: (s64)(1 << 63) == overflow And "shifted across entire bitwidth" is correctly detected: test_overflow: ok: (u8)(0 << 8) == overflow test_overflow: ok: (u16)(0 << 16) == overflow test_overflow: ok: (u32)(0 << 32) == overflow test_overflow: ok: (u64)(0 << 64) == overflow test_overflow: ok: (s8)(0 << 8) == overflow test_overflow: ok: (s16)(0 << 16) == overflow test_overflow: ok: (s32)(0 << 32) == overflow test_overflow: ok: (s64)(0 << 64) == overflow However, should these also be expected to fail, since they've shifted across their entire non-signed bits, or should it only fail if it tries the full type width? test_overflow: expected (s8)(0 << 7) to overflow test_overflow: expected (s16)(0 << 15) to overflow test_overflow: expected (int)(0 << 31) to overflow test_overflow: expected (s32)(0 << 31) to overflow test_overflow: expected (s64)(0 << 63) to overflow i.e. u8 0 << 8 and s8 with 0 << 8 already fail, but should s8 0 << 7 fail too? -Kees -- Kees Cook Pixel Security -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
