From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel De Graaf Subject: Re: [PATCH 3/8] libflask: Add boolean manipulation functions Date: Thu, 02 Feb 2012 10:22:16 -0500 Message-ID: <4F2AAA28.1030707@tycho.nsa.gov> References: <1328045178-30665-1-git-send-email-dgdegra@tycho.nsa.gov> <1328123365-12490-1-git-send-email-dgdegra@tycho.nsa.gov> <1328123365-12490-4-git-send-email-dgdegra@tycho.nsa.gov> <1328173562.17444.108.camel@zakaz.uk.xensource.com> <4F2A9DA4.7090500@tycho.nsa.gov> <1328194212.2924.22.camel@zakaz.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1328194212.2924.22.camel@zakaz.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: Ian Campbell Cc: "xen-devel@lists.xensource.com" List-Id: xen-devel@lists.xenproject.org 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 >>>> --- >>>> 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.