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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

>>> --- 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.

Jan