Re: New MISRA C:2012 R10.3 violations due to XSA-438 fix

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 20/09/2023 5:25 pm, Nicola Vetrini wrote:
> Hi all,
> 
> In light of the recent changes introduced by commit 
> fb0ff49fe9f784bfee0370c2a3c5f20e39d7a1cb,
> as part of XSA-438 [1], the signature of function pointer 'update_cr3' in
> 'xen/arch/x86/include/asm/paging.h', line 121, changed its second parameter 
> to a boolean.
> Consequently, all calls to that function should use a boolean to comply with 
> MISRA C:2012
> Rule 10.3 ("The value of an expression shall not be assigned to an object 
> with a narrower
> essential type or of a different essential type category"), but they still 
> use integers.
> These were the ones I can find:
> xen/arch/x86/include/asm/paging.h:299:    return 
> paging_get_hostmode(v)->update_cr3(v, 1, noflush);
> xen/arch/x86/mm/hap/hap.c:797:    hap_update_cr3(v, 0, false);
> xen/arch/x86/mm/shadow/common.c:2513:    v->arch.paging.mode->update_cr3(v, 
> 0, false);
> xen/arch/x86/mm/shadow/multi.c:2478:        
> v->arch.paging.mode->update_cr3(v, 0, false);
> 
> [1]
> https://lore.kernel.org/xen-devel/E1qitNQ-0001Pu-0K@xxxxxxxxxxxxxxxxxxxxxx/
> 

Hmm yes - the int->bool conversion is not relevant to the rest of the
change, and shouldn't have been part of a security fix even before
taking MISRA into consideration.

I truly detest APIs like this - whether it's 0/1 or true/false, the
callsites are unreadable due to the obfuscation.

However, Xen even has paging_lock_recursive() for the cases where the
paging lock may or may not be held, and needs to be.  So I'm fairly sure
the do_locking parameter is bogus in the first place.

~Andrew