Re: [PATCH 17/18] xenstored: add --priv-domid parameter

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

On 01/18/2012 06:48 AM, Ian Campbell wrote:
> On Thu, 2012-01-12 at 23:35 +0000, Daniel De Graaf wrote:
>> This parameter identifies an alternative service domain which has
>> superuser access to the xenstore database, which is currently required
>> to set up a new domain's xenstore entries.
> 
> Is this equivalent to dom0 adding write permissions to various paths for
> that domain as it builds it or is there more to it than that.
> 
> I know that the determination of "various paths" is non-trivial, so I'm
> not actually suggesting that is a better approach.
> 

It's more: the domain builder needs to create entries owned by the new
domain, and similar to UNIX chown() can only be called by the superuser.
The domain builder also currently relies on the fact that new keys it
creates inherit the parent's ownership instead of being owned by dom0.
The introduce operation is also privileged.

>>
>> Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
>> ---
>>  tools/xenstore/xenstored_core.c   |    5 +++++
>>  tools/xenstore/xenstored_core.h   |    1 +
>>  tools/xenstore/xenstored_domain.c |    2 +-
>>  3 files changed, 7 insertions(+), 1 deletions(-)
>>
>> diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
>> index eea5fd6..9d087de 100644
>> --- a/tools/xenstore/xenstored_core.c
>> +++ b/tools/xenstore/xenstored_core.c
>> @@ -1774,6 +1774,7 @@ static struct option options[] = {
>>  	{ "event", 1, NULL, 'e' },
>>  	{ "help", 0, NULL, 'H' },
>>  	{ "no-fork", 0, NULL, 'N' },
>> +	{ "priv-domid", 1, NULL, 'p' },
>>  	{ "output-pid", 0, NULL, 'P' },
>>  	{ "entry-size", 1, NULL, 'S' },
>>  	{ "trace-file", 1, NULL, 'T' },
>> @@ -1786,6 +1787,7 @@ static struct option options[] = {
>>  
>>  extern void dump_conn(struct connection *conn); 
>>  int dom0_event = 0;
>> +int priv_domid = 0;
>>  
>>  int main(int argc, char *argv[])
>>  {
>> @@ -1852,6 +1854,9 @@ int main(int argc, char *argv[])
>>  		case 'e':
>>  			dom0_event = strtol(optarg, NULL, 10);
>>  			break;
>> +		case 'p':
>> +			priv_domid = strtol(optarg, NULL, 10);
>> +			break;
>>  		}
>>  	}
>>  	if (optind != argc)
>> diff --git a/tools/xenstore/xenstored_core.h b/tools/xenstore/xenstored_core.h
>> index d3040ba..03e2e48 100644
>> --- a/tools/xenstore/xenstored_core.h
>> +++ b/tools/xenstore/xenstored_core.h
>> @@ -169,6 +169,7 @@ void dtrace_io(const struct connection *conn, const struct buffered_data *data,
>>  
>>  extern int event_fd;
>>  extern int dom0_event;
>> +extern int priv_domid;
>>  
>>  /* Map the kernel's xenstore page. */
>>  void *xenbus_map(void);
>> diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c
>> index 5f4a09e..46bcf3e 100644
>> --- a/tools/xenstore/xenstored_domain.c
>> +++ b/tools/xenstore/xenstored_domain.c
>> @@ -241,7 +241,7 @@ bool domain_can_read(struct connection *conn)
>>  
>>  bool domain_is_unprivileged(struct connection *conn)
>>  {
>> -	return (conn && conn->domain && conn->domain->domid != 0);
>> +	return (conn && conn->domain && conn->domain->domid != 0 && conn->domain->domid != priv_domid);
>>  }
>>  
>>  bool domain_can_write(struct connection *conn)
> 


-- 
Daniel De Graaf
National Security Agency