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

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

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.

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.

> Is there a defined maximum for the length of "name"?

INITCONTEXTLEN = 256.