Re: [PATCH for-4.18] x86/time: Fix UBSAN failure in __update_vcpu_system_time()

[Jan Beulich <jbeulich@xxxxxxxx>]

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