On 19.03.2024 04:34, Stefano Stabellini wrote: > On Mon, 18 Mar 2024, Jan Beulich wrote: >> On 16.03.2024 01:43, Stefano Stabellini wrote: >>> 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). >> >> You have a point here. I think though that at the very least the lost >> checking opportunity wants calling out quite explicitly. > > All right, then maybe this patch can go in with a clarification in the > commit message? > > Something like: > > Note that with SAF-5-safe in place, failures to have proper guards later > in the header files will not be reported
That would be okay with me. Jan
