Re: [PATCH 3/8] libflask: Add boolean manipulation functions

[Daniel De Graaf <dgdegra@tycho.nsa.gov>]

On 02/02/2012 09:50 AM, Ian Campbell wrote:
> On Thu, 2012-02-02 at 14:28 +0000, Daniel De Graaf wrote:
>> On 02/02/2012 04:06 AM, Ian Campbell wrote:
>>> On Wed, 2012-02-01 at 19:09 +0000, Daniel De Graaf wrote:
>>>> Add wrappers for getting and setting policy booleans by name or ID.
>>>>
>>>> Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
>>>> ---
>>>>  tools/flask/libflask/flask_op.c         |   59 +++++++++++++++++++++++++++++++
>>>>  tools/flask/libflask/include/libflask.h |    3 ++
>>>>  2 files changed, 62 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/tools/flask/libflask/flask_op.c b/tools/flask/libflask/flask_op.c
>>>> index d4b8ef0..412a05d 100644
>>>> --- a/tools/flask/libflask/flask_op.c
>>>> +++ b/tools/flask/libflask/flask_op.c
>>>> @@ -109,6 +109,65 @@ int flask_setenforce(xc_interface *xc_handle, int mode)
>>>>      return 0;
>>>>  }
>>>>  
>>>> +int flask_getbool_byid(xc_interface *xc_handle, int id, char *name, int *curr, int *pend)
>>>> +{
>>>> +    flask_op_t op;
>>>> +    char buf[255];
>>>> +    int rv;
>>>> +
>>>> +    op.cmd = FLASK_GETBOOL2;
>>>> +    op.buf = buf;
>>>> +    op.size = 255;
>>>
>>> sizeof(buf)? Here and elsewhere (including a few existing locations in
>>> flask_op.c).
>>>
>>>> +
>>>> +    snprintf(buf, sizeof buf, "%i", id);
>>>> +
>>>> +    rv = xc_flask_op(xc_handle, &op);
>>>> +
>>>> +    if ( rv )
>>>> +        return rv;
>>>> +    
>>>> +    sscanf(buf, "%i %i %s", curr, pend, name);
>>>
>>> Do you care about sscanf failures?
>>  
>> A failure here would be a sign of the hypervisor having made a format change
>> that is not backwards compatible. Checking it would be more complete, however.
>>
>>> It seems from other uses in the file that buf can contain binary data so
>>> would it make sense to make this two ints as binary followed by a
>>> string? That would remove string parsing here and in the hypervisor
>>> (which seems more critical to me?)
>>
>> That also seems far simpler to me; however, all the current FLASK hypercalls
>> are done via string parsing so deviating from this for new operations would
>> make them inconsistent.
> 
> OK. I thought I'd seen some binary muddling in their but I must have
> been mistaken.

Loading a policy seems to be the only operation not involving scanf.

>> If we didn't have to care about backwards compatibility I would convert the
>> entire flask_op hypercall to use a union-of-structures similar to domctl
>> because the string parsing introduces unneeded complexity.
> 
> How much do we care about backwards compat for this interface? Isn't it
> a tools only dom0 interface?
 
Looking over the users - yes, it is, so we should be fine breaking compat
here. This would also eliminate all in-hypervisor users of *scanf. I'll try
and see what a patch fixing this would look like.

I also noticed that libxc and libflask have parallel implementations of some
funcitons: xc_flask_getenforce from libxc and flask_getenforce from libflask.
Not sure if this is a leftover from before ACM support was removed, but it
appears that libflask can be eliminated completely (or remain only as a shim
to call libxc functions).

>>> Is there a defined maximum for the length of "name"?
>>
>> INITCONTEXTLEN = 256.
> 
> So the max size of the buffer is 256 + whatever two int and two spaces
> might maximally take, but your buffer is exactly 256.
> 

Agreed, it would be better to adjust this to a larger buffer.