| Message ID | 20170404191017.19584-4-konrad.wilk@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 04.04.17 at 21:10, <konrad.wilk@oracle.com> wrote: > @@ -1530,7 +1529,8 @@ int do_tmem_new_pool(domid_t this_cli_id, > pool->shared = 0; > goto out; > } > - if ( client->shared_auth_required && !tmem_global.shared_auth ) > + /* By default only join domains that are authorized by admin. */ > + if ( !tmem_global.shared_auth ) Why "by default"? Is this comment really useful here? Other than that the patch looks okay, but I won't claim to understand enough of tmem to know this is sufficiently backwards compatible, so I won't claim to have reviewed it in full. Jan
On Wed, Apr 05, 2017 at 03:36:51AM -0600, Jan Beulich wrote: > >>> On 04.04.17 at 21:10, <konrad.wilk@oracle.com> wrote: > > @@ -1530,7 +1529,8 @@ int do_tmem_new_pool(domid_t this_cli_id, > > pool->shared = 0; > > goto out; > > } > > - if ( client->shared_auth_required && !tmem_global.shared_auth ) > > + /* By default only join domains that are authorized by admin. */ > > + if ( !tmem_global.shared_auth ) > > Why "by default"? Is this comment really useful here? Other than Took the comment out. > that the patch looks okay, but I won't claim to understand enough > of tmem to know this is sufficiently backwards compatible, so I > won't claim to have reviewed it in full. The old clients that used shared pools work just fine. That is as long as the system admin invokes: xl tmem-shared-auth -u 00000000-0000-0000-0000-0000deadbeef -A 1 <domain> before hand (this is for UUID 0:deadbeef). [And to be honest the API is a bit weird - if you can't join a shared pool then you still get to join a private pool without any errors?!] Before this change you didn't have to invoke this tmem-shared-auth and any guest could join a shared pool, even malicious ones. From that perspective I did break backwards compatibility, but fixed a security hole. But as said - the guest won't notice - if the system admin didn't invoke the tmem-shared-auth - the hypervisor will gladly create another pool for them, it just that it won't be shared. > > Jan >
>>> On 05.04.17 at 15:40, <konrad.wilk@oracle.com> wrote: > On Wed, Apr 05, 2017 at 03:36:51AM -0600, Jan Beulich wrote: >> >>> On 04.04.17 at 21:10, <konrad.wilk@oracle.com> wrote: >> > @@ -1530,7 +1529,8 @@ int do_tmem_new_pool(domid_t this_cli_id, >> > pool->shared = 0; >> > goto out; >> > } >> > - if ( client->shared_auth_required && !tmem_global.shared_auth ) >> > + /* By default only join domains that are authorized by admin. */ >> > + if ( !tmem_global.shared_auth ) >> >> Why "by default"? Is this comment really useful here? Other than > > Took the comment out. >> that the patch looks okay, but I won't claim to understand enough >> of tmem to know this is sufficiently backwards compatible, so I >> won't claim to have reviewed it in full. > > The old clients that used shared pools work just fine. That is as long > as the system admin invokes: > xl tmem-shared-auth -u 00000000-0000-0000-0000-0000deadbeef -A 1 <domain> > > before hand (this is for UUID 0:deadbeef). > [And to be honest the API is a bit weird - if you can't join a shared > pool then you still get to join a private pool without any errors?!] > > > Before this change you didn't have to invoke this tmem-shared-auth > and any guest could join a shared pool, even malicious ones. > From that perspective I did break backwards compatibility, but fixed > a security hole. > > But as said - the guest won't notice - if the system admin didn't invoke > the tmem-shared-auth - the hypervisor will gladly create another pool > for them, it just that it won't be shared. Oh, that's even better than I had expected. Jan
diff --git a/docs/misc/tmem-internals.html b/docs/misc/tmem-internals.html index 2d8635d..9b7e70e 100644 --- a/docs/misc/tmem-internals.html +++ b/docs/misc/tmem-internals.html @@ -715,11 +715,9 @@ Ocfs2 has only very limited security; it is assumed that anyone who can access the filesystem bits on the shared disk can mount the filesystem and use it. But in a virtualized data center, higher isolation requirements may apply. -As a result, a Xen boot option -- "tmem_shared_auth" -- was -added. The option defaults to disabled, -but when it is enabled, management tools must explicitly authenticate (or may +As a result, management tools must explicitly authenticate (or may explicitly deny) shared pool access to any client. -On Xen, this is done with the "xm +On Xen, this is done with the "xl tmem-shared-auth" command. <P> <b><i>32-bit implementation</i>.</b> diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown index 9eb85d6..9690512 100644 --- a/docs/misc/xen-command-line.markdown +++ b/docs/misc/xen-command-line.markdown @@ -1539,9 +1539,6 @@ pages) must also be specified via the tbuf\_size parameter. ### tmem\_compress > `= <boolean>` -### tmem\_shared\_auth -> `= <boolean>` - ### tsc > `= unstable | skewed | stable:socket` diff --git a/xen/common/tmem.c b/xen/common/tmem.c index 158d660..4f77a8c 100644 --- a/xen/common/tmem.c +++ b/xen/common/tmem.c @@ -846,7 +846,6 @@ struct client *client_create(domid_t cli_id) client->info.version = TMEM_SPEC_VERSION; client->info.maxpools = MAX_POOLS_PER_DOMAIN; client->info.flags.u.compress = tmem_compression_enabled(); - client->shared_auth_required = tmem_shared_auth(); for ( i = 0; i < MAX_GLOBAL_SHARED_POOLS; i++) client->shared_auth_uuid[i][0] = client->shared_auth_uuid[i][1] = -1L; @@ -1530,7 +1529,8 @@ int do_tmem_new_pool(domid_t this_cli_id, pool->shared = 0; goto out; } - if ( client->shared_auth_required && !tmem_global.shared_auth ) + /* By default only join domains that are authorized by admin. */ + if ( !tmem_global.shared_auth ) { for ( i = 0; i < MAX_GLOBAL_SHARED_POOLS; i++) if ( (client->shared_auth_uuid[i][0] == uuid_lo) && diff --git a/xen/common/tmem_xen.c b/xen/common/tmem_xen.c index 7d60b71..06ce3ef 100644 --- a/xen/common/tmem_xen.c +++ b/xen/common/tmem_xen.c @@ -20,9 +20,6 @@ boolean_param("tmem", opt_tmem); bool_t __read_mostly opt_tmem_compress = 0; boolean_param("tmem_compress", opt_tmem_compress); -bool_t __read_mostly opt_tmem_shared_auth = 0; -boolean_param("tmem_shared_auth", opt_tmem_shared_auth); - atomic_t freeable_page_count = ATOMIC_INIT(0); /* these are a concurrency bottleneck, could be percpu and dynamically diff --git a/xen/include/xen/tmem_xen.h b/xen/include/xen/tmem_xen.h index 70cc108..dc5888c 100644 --- a/xen/include/xen/tmem_xen.h +++ b/xen/include/xen/tmem_xen.h @@ -41,12 +41,6 @@ static inline bool_t tmem_compression_enabled(void) return opt_tmem_compress; } -extern bool_t opt_tmem_shared_auth; -static inline bool_t tmem_shared_auth(void) -{ - return opt_tmem_shared_auth; -} - #ifdef CONFIG_TMEM extern bool_t opt_tmem; static inline bool_t tmem_enabled(void) @@ -291,7 +285,6 @@ struct client { long eph_count, eph_count_max; domid_t cli_id; xen_tmem_client_t info; - bool_t shared_auth_required; /* For save/restore/migration. */ bool_t was_frozen; struct list_head persistent_invalidated_list;
Having an off by default option allowing guests to join _any_ shared pool is not very secure. Lets eliminate tmem_shared_auth bootup option (which was disabled by default) and have the code force this by default. Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> --- docs/misc/tmem-internals.html | 6 ++---- docs/misc/xen-command-line.markdown | 3 --- xen/common/tmem.c | 4 ++-- xen/common/tmem_xen.c | 3 --- xen/include/xen/tmem_xen.h | 7 ------- 5 files changed, 4 insertions(+), 19 deletions(-)