Re: [PATCH 9/9] xen: add SAF deviation for safe cast removal.

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 15 Dec 2023, Jan Beulich wrote:
> On 14.12.2023 23:04, Stefano Stabellini wrote:
> > On Thu, 14 Dec 2023, Jan Beulich wrote:
> >> On 14.12.2023 13:07, Simone Ballarin wrote:
> >>> --- a/docs/misra/safe.json
> >>> +++ b/docs/misra/safe.json
> >>> @@ -28,6 +28,14 @@
> >>>          },
> >>>          {
> >>>              "id": "SAF-3-safe",
> >>> +            "analyser": {
> >>> +                "eclair": "MC3R1.R11.8"
> >>> +            },
> >>> +            "name": "MC3R1.R11.8: removal of const qualifier to comply 
> >>> with function signature",
> >>> +            "text": "It is safe to cast away const qualifiers to comply 
> >>> with function signature if the function does not modify the pointee."
> >>
> >> I'm not happy with this description, as it invites for all sorts of abuse.
> >> Yet I'm also puzzled that ...
> > 
> > We can improve the language but the concept would still be the same. For
> > instance:
> > 
> > A single function might or might not modify the pointee depending on
> > other function parameters (for instance a single function that could
> > either read or write depending on how it is called). It is safe to cast
> > away const qualifiers when passing a parameter to a function of this
> > type when the other parameters are triggering a read-only operation.
> 
> Right, but I think the next here needs to be setting as tight boundaries
> as possible: It should cover only this one specific pattern. Anything
> else would better get its own deviation, imo.

OK. What about:

A single function might or might not modify the pointee depending on
other function parameters, for instance a common pattern is to implement
one function that could either read or write depending on how it is
called. It is safe to cast away const qualifiers when passing a
parameter to a function following this pattern when the other parameters
are triggering a read-only operation.

Feel free to suggest a better wording.


> >>> --- a/xen/arch/x86/hvm/hvm.c
> >>> +++ b/xen/arch/x86/hvm/hvm.c
> >>> @@ -3413,6 +3413,7 @@ static enum hvm_translation_result __hvm_copy(
> >>>  enum hvm_translation_result hvm_copy_to_guest_phys(
> >>>      paddr_t paddr, const void *buf, unsigned int size, struct vcpu *v)
> >>>  {
> >>> +    /* SAF-3-safe */
> >>>      return __hvm_copy((void *)buf /* HVMCOPY_to_guest doesn't modify */,
> >>>                        paddr, size, v,
> >>>                        HVMCOPY_to_guest | HVMCOPY_phys, 0, NULL);
> >>
> >> ... this is the only place you then use it. Afaict some of Arm's 
> >> copy_guest()
> >> callers ought to have a similar issue. If so, an enlarged patch should be
> >> discussed with a larger audience, to see how we collectively think we want 
> >> to
> >> put this specific kind of deviation.
> > 
> > We have a similar problem, see xen/arch/arm/guestcopy.c
> > raw_copy_to_guest and raw_copy_from_guest.
> > 
> > I would use the SAF deviation there too.
> > 
> > In the case here, I think the comment "HVMCOPY_to_guest doesn't modify"
> > could be removed as it becomes redundant with SAF-3-safe, but I'll leave
> > it to you.
> 
> No, the comment cannot be removed: The SAF comment says exactly nothing
> until you go and look up its description. The two comments could be
> folded, though. Which is something I was trying to advocate for in
> general: Unless entirely obvious, what exactly it is that is "safe"
> would better be (briefly) stated in these SAF comments.

I agree with you