ossfuzz: fix UB in Cr50 introduced by NVMEM size change |
|||
Issue descriptionPer https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=2326, cr50 hits undefined behavior after https://chromium-review.googlesource.com/433839. apronin's analysis: The offending code boils down to: UINT32 handle; if (handle >= (0x80 << 24) && handle <= ((0x80 << 24) + 3 - 1)) { ... } Iirc many compilers (a) will just preserve bits as is when storing 0x80 << 24 in signed int (even though > MAX_INT, it is a valid shift on a positive value + defined conversion to signed int, which keeps the bits), (b) cast int to unsigned (again, effectively keeping bits as is) before making this comparison. So, the resulting comparison will be correct. Will need to check for the compiler used for cr50. But probably should be fixed even if to avoid such warnings and potential undefined behavior anyways. vbendeb@ - since you made the change that cause this report, can I ask you to drive the fix?
,
Sep 20 2017
One thing that bothers me: I always thought that shift is done at compile time, not at run-time. So, how does the fuzzer detects an undefined shift at run-time - it's not a static analyzer? Will need to check with fuzzer experts.
,
Sep 25 2017
Good question in #2, Kostya, can you please comment? So, would the fix be just replacing 0x80 << 24 with 0x80000000? Or does 0x80000000 still require explicit typecast to Uint32? Pardon my ignorance, but how do I run the fuzzer?
,
Oct 9 2017
To capture discussions from a separate email thread: Re #2: the fuzzer build uses ubsan to flag bugs at run-time: https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html. Re #3: The specific simplest fix (suggested by kcc@) was: "... use 0x80U instead of 0x80" for TPM_HT_TRANSIENT, which makes sense. Same with 0x81u for TPM_HT_PERSISTENT and other Table 28 constants in tpm_types.h.
,
Jul 20
this seems not to be a problem any more and the fuzzer bug is marked 'fixed' |
|||
►
Sign in to add a comment |
|||
Comment 1 by mnissler@chromium.org
, Sep 19 2017