On Fri, 15 Mar 2024, Jan Beulich wrote: > On 14.03.2024 23:59, Stefano Stabellini wrote: > > On Mon, 11 Mar 2024, Simone Ballarin wrote: > >> On 11/03/24 14:56, Jan Beulich wrote: > >>> On 11.03.2024 13:00, Simone Ballarin wrote: > >>>> On 11/03/24 11:08, Jan Beulich wrote: > >>>>> On 11.03.2024 09:59, Simone Ballarin wrote: > >>>>>> --- a/xen/arch/arm/include/asm/hypercall.h > >>>>>> +++ b/xen/arch/arm/include/asm/hypercall.h > >>>>>> @@ -1,3 +1,4 @@ > >>>>>> +/* SAF-5-safe direct inclusion guard before */ > >>>>>> #ifndef __XEN_HYPERCALL_H__ > >>>>>> #error "asm/hypercall.h should not be included directly - include > >>>>>> xen/hypercall.h instead" > >>>>>> #endif > >>>>>> --- a/xen/arch/x86/include/asm/hypercall.h > >>>>>> +++ b/xen/arch/x86/include/asm/hypercall.h > >>>>>> @@ -2,6 +2,7 @@ > >>>>>> * asm-x86/hypercall.h > >>>>>> */ > >>>>>> +/* SAF-5-safe direct inclusion guard before */ > >>>>>> #ifndef __XEN_HYPERCALL_H__ > >>>>>> #error "asm/hypercall.h should not be included directly - include > >>>>>> xen/hypercall.h instead" > >>>>>> #endif > >>>>> > >>>>> Iirc it was said that this way checking for correct guards is suppressed > >>>>> altogether in Eclair, which is not what we want. Can you clarify this, > >>>>> please? > >>>>> > >>>> > >>>> My first change was moving this check inside the guard. > >>>> You commented my patch saying that this would be an error because someone > >>>> can > >>>> include it directly if it has already been included indirectly. > >>>> I replied telling that this was the case also before the change. > >>>> You agreed with me, and we decided that the correct thing would be fixing > >>>> the > >>>> check and not apply my temporary change to address the finding. > >>>> > >>>> Considering that the code should be amended, a SAF deviation seems to me > >>>> the most appropriate way for suppressing these findings. > >>> > >>> Since I don't feel your reply addresses my question, asking differently: > >>> With > >>> your change in place, will failure to have proper guards (later) in these > >>> headers still be reported by Eclair? > >> > >> No, if you put something between the check and the guard, > >> no violation will be reported. > > > > From this email exchange I cannot under if Jan is OK with this patch or > > not. > > Whether I'm okay(ish) with the patch here depends on our position towards > the lost checking in Eclair mentioned above. To me it still looks relevant > that checking for a guard occurs, even if that isn't first in a file for > some specific reason.
More checking is better than less checking, but if we cannot find a simple and actionable way forward on this violation, deviating it is still a big improvement because it allows us to enable the ECLAIR Dir 4.10 checks in xen.git overall (which again goes back to more checking is better than less checking). Do you have a simple alternative suggestion? If not, this is still an improvement.
