Re: [PATCH v5 03/10] conf: introduce launch-security element in domain

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




[...]

>>
>> Hopefully hexuint will suffice over time... On the other hand, this
>> patch uses virXPathULongHex in order to parse.
>>
> 
> IIRC, I was not able to find anything other than hexuint in
> basictypes.rng and also was not able to a function like
> virXPathUIntHex(..). If you can point me to the function which can be
> used to get the hexuint then I am good with it. Also, I am open to
> adding such function if it does not exist and anyone else sees the need
> for it.
> 
> 

Right - if they need to be created, then you can do so.

I think in the long run since field is defined as a 32 bit unsigned
quantity, then we should be OK with sticking with that. If you need to
invent a 'hexulong' which is a slight change from 'hexuint' that's also
a possibility.

OTOH: if 32 bits are fine, it's "OK" to use the virXPathULongHex as long
as you also test that the result isn't longer than 32 bits. I wouldn't
bother with a virXPathUIntHex API since in the long run all it "should"
do is call the Long one and do the max uint comparison.

> 
>>> +        <optional>
>>> +          <element name="handle">
>>> +            <ref name='unsignedInt'/>
>>> +          </element>
>>> +        </optional>
>>> +        <optional>
>>> +          <element name="dh-cert">
>>> +            <data type="string"/>
>>> +          </element>
>>> +        </optional>
>>> +        <optional>
>>> +          <element name="session">
>>> +            <data type="string"/>
>>> +          </element>
>>> +        </optional>
>>> +      </interleave>
>>> +    </element>
>>> +  </define>
>>> +

[...]

>>> +
>>> +    if (virXPathULongHex("string(./policy)", ctxt, &policy) < 0)>
>>> +        policy = 0x1;
>>
>> Hmmm... This one is optional which makes things a bit interesting. How
>> do you know it was provided? Today the default could be 0x1, but
>> sometime in the future if you decide upon a different default, then
>> things get really complicated. Or for some other chip and/or hypervisor
>> the default won't be 1.
>>
> 
> Firmware does not have default policy, a caller must provide the policy
> value. In our cases, we are saying if caller does not provide a policy
> in xml then we default to 0x1.
> 

As noted in my 6/10 response - you could change to making policy
required and then not worry about a default. It is a gray area, but it
will be in the long run some sort of hypervisor and even chip dependent
type field.

>> Also virXPathULongHex returns -1 or -2 w/ different meanings - if it's
>> not there, You get a -1 when not provided and -2 when you have invalid
>> value in field, which should be an error.
>>
> 
> ah thats good information, I was not aware of -2 thing.
> 
> 
>> Finally, ULongHex returns 64 bits and your field is 32 bits leading to
>> possible overflow
>>
> 
> Right, this is where I was struggling because there is no such function
> as virXPathUIntHex(...) which ca be used to get uint32_t. Please let me
> know your thoughts on  how do you want me to handle this situation.
> 

See my note above - I think the code below covers the UINT_MAX case
while using the ULong API.

Obviously if you make policy required, then the code changes a bit to
get/return rc and check rc vs. -1 for not found and vs. -2 for found,
but invalid (common occurrence elsewhere).

John

> 
> 
> 
>> So, either you have to have a "policy_provided" boolean of some sort or
>> I think preferably leave it as 0 (zero) indicating not provided and then
>> when generating a command line check against 0 and provide the
>> hypervisor dependent value on the command line *OR* just don't provided
>> it an let the hypervisor choose it's default value because the value
>> wasn't provided (that's a later patch though)
>>
>> Also see [1] below...
>>
>> So my suggestion is:
>>
>>      if (virXPathULongHex("string(./policy)", ctxt, &policy) == -2 ||
>>          policy > UINT_MAX) {
>>          virReportError(VIR_ERR_XML_ERROR, "%s",
>>                         _("invalid launch-security policy value"));
>>          goto error;
>>      }
>>      def->policy = policy;
>>
>> If -1 is returned, the def->policy = 0; otherwise, it's set to something.
>>

[...]

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux