On 02.11.2023 20:05, Andrew Cooper wrote: > On 02/11/2023 8:33 am, Jan Beulich wrote: >> Furthermore, if you deem this XSA-worthy despite the generated code not >> resulting in any real misbehavior > > ... we've issued XSAs for this class of issue before. XSA-328 is the > one I specifically remember, but I'm sure we've done others too. > > In this case, an unprivileged guest can control the NULL-ness here, so > if there's a practical consequence from the compiler then the guest can > definitely tickle that consequence. > > Alternatively, the security team could decide to change it's stance on > this class of issues. > >> , code patterns like >> (found in free_heap_pages()) >> >> struct page_info *predecessor = pg - mask; >> >> /* Merge with predecessor block? */ >> if ( !mfn_valid(page_to_mfn(predecessor)) || >> >> or (found in get_page_from_l1e()) >> >> struct page_info *page = mfn_to_page(_mfn(mfn)); >> ... >> valid = mfn_valid(_mfn(mfn)); >> >> if ( !valid || >> >> would be in the same class (access outside of array bounds), just that the >> checker cannot spot those without producing false positives (simply because >> it doesn't know frame_table[]'s bounds). We're fully aware of the existence >> of such code, and we've decided to - if at all - only eliminate such cases >> (slowly) as respective code is touched anyway. > > I don't agree with describing these as the same class. NULL deference > is different to OoB, even if they overlap from an underlying mechanics > point of view. > > Nevertheless, I've raised that "valid" pattern with the security team > before, and I would certainly prefer to express it differently. > > But neither of them trigger UBSAN. GCC can't see any wiggle room to > potentially optimise, and I expect it's because __mfn_valid() is in an > external call. > > If we had working LTO, I'd be interested to see that alters the UBSAN > determination or not.
I'm not convinced it takes as much as working LTO. With PDX_COMPRESSION=n I question __mfn_valid() needing to be an out-of-line function. Converting it (back) to an inline one would better not come with the risk of breaking certain use sites. Jan
