| Message ID | 20201002154141.11677-4-jgross@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | tools/xenstore: remove read-only socket | expand |
On Fri, Oct 02, 2020 at 05:41:39PM +0200, Juergen Gross wrote: > Today it is possible to open the connection in read-only mode via > xs_daemon_open_readonly(). This is working only with Xenstore running > as a daemon in the same domain as the user. Additionally it doesn't > add any security as accessing the socket used for that functionality > requires the same privileges as the socket used for full Xenstore > access. > > So just drop the read-only semantics in all cases, leaving the > interface existing only for compatibility reasons. This in turn > requires to just ignore the XS_OPEN_READONLY flag. > > Signed-off-by: Juergen Gross <jgross@suse.com> > --- > tools/libs/store/include/xenstore.h | 8 -------- > tools/libs/store/xs.c | 7 ++----- > 2 files changed, 2 insertions(+), 13 deletions(-) > > diff --git a/tools/libs/store/include/xenstore.h b/tools/libs/store/include/xenstore.h > index cbc7206a0f..158e69ef83 100644 > --- a/tools/libs/store/include/xenstore.h > +++ b/tools/libs/store/include/xenstore.h > @@ -60,15 +60,12 @@ typedef uint32_t xs_transaction_t; > /* Open a connection to the xs daemon. > * Attempts to make a connection over the socket interface, > * and if it fails, then over the xenbus interface. > - * Mode 0 specifies read-write access, XS_OPEN_READONLY for > - * read-only access. > * > * * Connections made with xs_open(0) (which might be shared page or > * socket based) are only guaranteed to work in the parent after > * fork. > * * xs_daemon_open*() and xs_domain_open() are deprecated synonyms > * for xs_open(0). > - * * XS_OPEN_READONLY has no bearing on any of this. > * > * Returns a handle or NULL. > */ > @@ -83,11 +80,6 @@ void xs_close(struct xs_handle *xsh /* NULL ok */); > */ > struct xs_handle *xs_daemon_open(void); > struct xs_handle *xs_domain_open(void); > - > -/* Connect to the xs daemon (readonly for non-root clients). > - * Returns a handle or NULL. > - * Deprecated, please use xs_open(XS_OPEN_READONLY) instead > - */ > struct xs_handle *xs_daemon_open_readonly(void); > > /* Close the connection to the xs daemon. > diff --git a/tools/libs/store/xs.c b/tools/libs/store/xs.c > index 320734416f..4ac73ec317 100644 > --- a/tools/libs/store/xs.c > +++ b/tools/libs/store/xs.c > @@ -302,7 +302,7 @@ struct xs_handle *xs_daemon_open(void) > > struct xs_handle *xs_daemon_open_readonly(void) > { > - return xs_open(XS_OPEN_READONLY); > + return xs_open(0); > } This changes the semantics of this function, isn't it? In that the user expects a readonly connection but in fact a read-write connection is returned. Maybe we should return an error here? Wei.
On 07.10.20 12:54, Wei Liu wrote: > On Fri, Oct 02, 2020 at 05:41:39PM +0200, Juergen Gross wrote: >> Today it is possible to open the connection in read-only mode via >> xs_daemon_open_readonly(). This is working only with Xenstore running >> as a daemon in the same domain as the user. Additionally it doesn't >> add any security as accessing the socket used for that functionality >> requires the same privileges as the socket used for full Xenstore >> access. >> >> So just drop the read-only semantics in all cases, leaving the >> interface existing only for compatibility reasons. This in turn >> requires to just ignore the XS_OPEN_READONLY flag. >> >> Signed-off-by: Juergen Gross <jgross@suse.com> >> --- >> tools/libs/store/include/xenstore.h | 8 -------- >> tools/libs/store/xs.c | 7 ++----- >> 2 files changed, 2 insertions(+), 13 deletions(-) >> >> diff --git a/tools/libs/store/include/xenstore.h b/tools/libs/store/include/xenstore.h >> index cbc7206a0f..158e69ef83 100644 >> --- a/tools/libs/store/include/xenstore.h >> +++ b/tools/libs/store/include/xenstore.h >> @@ -60,15 +60,12 @@ typedef uint32_t xs_transaction_t; >> /* Open a connection to the xs daemon. >> * Attempts to make a connection over the socket interface, >> * and if it fails, then over the xenbus interface. >> - * Mode 0 specifies read-write access, XS_OPEN_READONLY for >> - * read-only access. >> * >> * * Connections made with xs_open(0) (which might be shared page or >> * socket based) are only guaranteed to work in the parent after >> * fork. >> * * xs_daemon_open*() and xs_domain_open() are deprecated synonyms >> * for xs_open(0). >> - * * XS_OPEN_READONLY has no bearing on any of this. >> * >> * Returns a handle or NULL. >> */ >> @@ -83,11 +80,6 @@ void xs_close(struct xs_handle *xsh /* NULL ok */); >> */ >> struct xs_handle *xs_daemon_open(void); >> struct xs_handle *xs_domain_open(void); >> - >> -/* Connect to the xs daemon (readonly for non-root clients). >> - * Returns a handle or NULL. >> - * Deprecated, please use xs_open(XS_OPEN_READONLY) instead >> - */ >> struct xs_handle *xs_daemon_open_readonly(void); >> >> /* Close the connection to the xs daemon. >> diff --git a/tools/libs/store/xs.c b/tools/libs/store/xs.c >> index 320734416f..4ac73ec317 100644 >> --- a/tools/libs/store/xs.c >> +++ b/tools/libs/store/xs.c >> @@ -302,7 +302,7 @@ struct xs_handle *xs_daemon_open(void) >> >> struct xs_handle *xs_daemon_open_readonly(void) >> { >> - return xs_open(XS_OPEN_READONLY); >> + return xs_open(0); >> } > > This changes the semantics of this function, isn't it? In that the user > expects a readonly connection but in fact a read-write connection is > returned. > > Maybe we should return an error here? No, as the behavior is this way already if any of the following is true: - a guest is opening the connection - Xenstore is running in a stubdom - oxenstored is being used Juergen
On 07/10/2020 11:57, Jürgen Groß wrote: > On 07.10.20 12:54, Wei Liu wrote: >> On Fri, Oct 02, 2020 at 05:41:39PM +0200, Juergen Gross wrote: >>> Today it is possible to open the connection in read-only mode via >>> xs_daemon_open_readonly(). This is working only with Xenstore running >>> as a daemon in the same domain as the user. Additionally it doesn't >>> add any security as accessing the socket used for that functionality >>> requires the same privileges as the socket used for full Xenstore >>> access. >>> >>> So just drop the read-only semantics in all cases, leaving the >>> interface existing only for compatibility reasons. This in turn >>> requires to just ignore the XS_OPEN_READONLY flag. >>> >>> Signed-off-by: Juergen Gross <jgross@suse.com> >>> --- >>> tools/libs/store/include/xenstore.h | 8 -------- >>> tools/libs/store/xs.c | 7 ++----- >>> 2 files changed, 2 insertions(+), 13 deletions(-) >>> >>> diff --git a/tools/libs/store/include/xenstore.h >>> b/tools/libs/store/include/xenstore.h >>> index cbc7206a0f..158e69ef83 100644 >>> --- a/tools/libs/store/include/xenstore.h >>> +++ b/tools/libs/store/include/xenstore.h >>> @@ -60,15 +60,12 @@ typedef uint32_t xs_transaction_t; >>> /* Open a connection to the xs daemon. >>> * Attempts to make a connection over the socket interface, >>> * and if it fails, then over the xenbus interface. >>> - * Mode 0 specifies read-write access, XS_OPEN_READONLY for >>> - * read-only access. >>> * >>> * * Connections made with xs_open(0) (which might be shared page or >>> * socket based) are only guaranteed to work in the parent after >>> * fork. >>> * * xs_daemon_open*() and xs_domain_open() are deprecated synonyms >>> * for xs_open(0). >>> - * * XS_OPEN_READONLY has no bearing on any of this. >>> * >>> * Returns a handle or NULL. >>> */ >>> @@ -83,11 +80,6 @@ void xs_close(struct xs_handle *xsh /* NULL ok */); >>> */ >>> struct xs_handle *xs_daemon_open(void); >>> struct xs_handle *xs_domain_open(void); >>> - >>> -/* Connect to the xs daemon (readonly for non-root clients). >>> - * Returns a handle or NULL. >>> - * Deprecated, please use xs_open(XS_OPEN_READONLY) instead >>> - */ >>> struct xs_handle *xs_daemon_open_readonly(void); >>> /* Close the connection to the xs daemon. >>> diff --git a/tools/libs/store/xs.c b/tools/libs/store/xs.c >>> index 320734416f..4ac73ec317 100644 >>> --- a/tools/libs/store/xs.c >>> +++ b/tools/libs/store/xs.c >>> @@ -302,7 +302,7 @@ struct xs_handle *xs_daemon_open(void) >>> struct xs_handle *xs_daemon_open_readonly(void) >>> { >>> - return xs_open(XS_OPEN_READONLY); >>> + return xs_open(0); >>> } >> >> This changes the semantics of this function, isn't it? In that the user >> expects a readonly connection but in fact a read-write connection is >> returned. >> >> Maybe we should return an error here? > > No, as the behavior is this way already if any of the following is true: > > - a guest is opening the connection > - Xenstore is running in a stubdom > - oxenstored is being used Right, and these are just some of the reasons why dropping the R/O socket is a sensible thing to do. However, I do think xenstore.h needs large disclaimers next to the relevant constants and functions that both XS_OPEN_* flags are now obsolete and ignored (merged into appropriate patches in the series). ~Andrew
On 07.10.20 13:50, Andrew Cooper wrote: > On 07/10/2020 11:57, Jürgen Groß wrote: >> On 07.10.20 12:54, Wei Liu wrote: >>> On Fri, Oct 02, 2020 at 05:41:39PM +0200, Juergen Gross wrote: >>>> Today it is possible to open the connection in read-only mode via >>>> xs_daemon_open_readonly(). This is working only with Xenstore running >>>> as a daemon in the same domain as the user. Additionally it doesn't >>>> add any security as accessing the socket used for that functionality >>>> requires the same privileges as the socket used for full Xenstore >>>> access. >>>> >>>> So just drop the read-only semantics in all cases, leaving the >>>> interface existing only for compatibility reasons. This in turn >>>> requires to just ignore the XS_OPEN_READONLY flag. >>>> >>>> Signed-off-by: Juergen Gross <jgross@suse.com> >>>> --- >>>> tools/libs/store/include/xenstore.h | 8 -------- >>>> tools/libs/store/xs.c | 7 ++----- >>>> 2 files changed, 2 insertions(+), 13 deletions(-) >>>> >>>> diff --git a/tools/libs/store/include/xenstore.h >>>> b/tools/libs/store/include/xenstore.h >>>> index cbc7206a0f..158e69ef83 100644 >>>> --- a/tools/libs/store/include/xenstore.h >>>> +++ b/tools/libs/store/include/xenstore.h >>>> @@ -60,15 +60,12 @@ typedef uint32_t xs_transaction_t; >>>> /* Open a connection to the xs daemon. >>>> * Attempts to make a connection over the socket interface, >>>> * and if it fails, then over the xenbus interface. >>>> - * Mode 0 specifies read-write access, XS_OPEN_READONLY for >>>> - * read-only access. >>>> * >>>> * * Connections made with xs_open(0) (which might be shared page or >>>> * socket based) are only guaranteed to work in the parent after >>>> * fork. >>>> * * xs_daemon_open*() and xs_domain_open() are deprecated synonyms >>>> * for xs_open(0). >>>> - * * XS_OPEN_READONLY has no bearing on any of this. >>>> * >>>> * Returns a handle or NULL. >>>> */ >>>> @@ -83,11 +80,6 @@ void xs_close(struct xs_handle *xsh /* NULL ok */); >>>> */ >>>> struct xs_handle *xs_daemon_open(void); >>>> struct xs_handle *xs_domain_open(void); >>>> - >>>> -/* Connect to the xs daemon (readonly for non-root clients). >>>> - * Returns a handle or NULL. >>>> - * Deprecated, please use xs_open(XS_OPEN_READONLY) instead >>>> - */ >>>> struct xs_handle *xs_daemon_open_readonly(void); >>>> /* Close the connection to the xs daemon. >>>> diff --git a/tools/libs/store/xs.c b/tools/libs/store/xs.c >>>> index 320734416f..4ac73ec317 100644 >>>> --- a/tools/libs/store/xs.c >>>> +++ b/tools/libs/store/xs.c >>>> @@ -302,7 +302,7 @@ struct xs_handle *xs_daemon_open(void) >>>> struct xs_handle *xs_daemon_open_readonly(void) >>>> { >>>> - return xs_open(XS_OPEN_READONLY); >>>> + return xs_open(0); >>>> } >>> >>> This changes the semantics of this function, isn't it? In that the user >>> expects a readonly connection but in fact a read-write connection is >>> returned. >>> >>> Maybe we should return an error here? >> >> No, as the behavior is this way already if any of the following is true: >> >> - a guest is opening the connection >> - Xenstore is running in a stubdom >> - oxenstored is being used > > Right, and these are just some of the reasons why dropping the R/O > socket is a sensible thing to do. > > However, I do think xenstore.h needs large disclaimers next to the > relevant constants and functions that both XS_OPEN_* flags are now > obsolete and ignored (merged into appropriate patches in the series). Fine with me. Juergen
On Wed, Oct 07, 2020 at 02:45:29PM +0200, Jürgen Groß wrote: > On 07.10.20 13:50, Andrew Cooper wrote: > > On 07/10/2020 11:57, Jürgen Groß wrote: > > > On 07.10.20 12:54, Wei Liu wrote: > > > > On Fri, Oct 02, 2020 at 05:41:39PM +0200, Juergen Gross wrote: > > > > > Today it is possible to open the connection in read-only mode via > > > > > xs_daemon_open_readonly(). This is working only with Xenstore running > > > > > as a daemon in the same domain as the user. Additionally it doesn't > > > > > add any security as accessing the socket used for that functionality > > > > > requires the same privileges as the socket used for full Xenstore > > > > > access. > > > > > > > > > > So just drop the read-only semantics in all cases, leaving the > > > > > interface existing only for compatibility reasons. This in turn > > > > > requires to just ignore the XS_OPEN_READONLY flag. > > > > > > > > > > Signed-off-by: Juergen Gross <jgross@suse.com> > > > > > --- > > > > > tools/libs/store/include/xenstore.h | 8 -------- > > > > > tools/libs/store/xs.c | 7 ++----- > > > > > 2 files changed, 2 insertions(+), 13 deletions(-) > > > > > > > > > > diff --git a/tools/libs/store/include/xenstore.h > > > > > b/tools/libs/store/include/xenstore.h > > > > > index cbc7206a0f..158e69ef83 100644 > > > > > --- a/tools/libs/store/include/xenstore.h > > > > > +++ b/tools/libs/store/include/xenstore.h > > > > > @@ -60,15 +60,12 @@ typedef uint32_t xs_transaction_t; > > > > > /* Open a connection to the xs daemon. > > > > > * Attempts to make a connection over the socket interface, > > > > > * and if it fails, then over the xenbus interface. > > > > > - * Mode 0 specifies read-write access, XS_OPEN_READONLY for > > > > > - * read-only access. > > > > > * > > > > > * * Connections made with xs_open(0) (which might be shared page or > > > > > * socket based) are only guaranteed to work in the parent after > > > > > * fork. > > > > > * * xs_daemon_open*() and xs_domain_open() are deprecated synonyms > > > > > * for xs_open(0). > > > > > - * * XS_OPEN_READONLY has no bearing on any of this. > > > > > * > > > > > * Returns a handle or NULL. > > > > > */ > > > > > @@ -83,11 +80,6 @@ void xs_close(struct xs_handle *xsh /* NULL ok */); > > > > > */ > > > > > struct xs_handle *xs_daemon_open(void); > > > > > struct xs_handle *xs_domain_open(void); > > > > > - > > > > > -/* Connect to the xs daemon (readonly for non-root clients). > > > > > - * Returns a handle or NULL. > > > > > - * Deprecated, please use xs_open(XS_OPEN_READONLY) instead > > > > > - */ > > > > > struct xs_handle *xs_daemon_open_readonly(void); > > > > > /* Close the connection to the xs daemon. > > > > > diff --git a/tools/libs/store/xs.c b/tools/libs/store/xs.c > > > > > index 320734416f..4ac73ec317 100644 > > > > > --- a/tools/libs/store/xs.c > > > > > +++ b/tools/libs/store/xs.c > > > > > @@ -302,7 +302,7 @@ struct xs_handle *xs_daemon_open(void) > > > > > struct xs_handle *xs_daemon_open_readonly(void) > > > > > { > > > > > - return xs_open(XS_OPEN_READONLY); > > > > > + return xs_open(0); > > > > > } > > > > > > > > This changes the semantics of this function, isn't it? In that the user > > > > expects a readonly connection but in fact a read-write connection is > > > > returned. > > > > > > > > Maybe we should return an error here? > > > > > > No, as the behavior is this way already if any of the following is true: > > > > > > - a guest is opening the connection > > > - Xenstore is running in a stubdom > > > - oxenstored is being used > > > > Right, and these are just some of the reasons why dropping the R/O > > socket is a sensible thing to do. > > > > However, I do think xenstore.h needs large disclaimers next to the > > relevant constants and functions that both XS_OPEN_* flags are now > > obsolete and ignored (merged into appropriate patches in the series). > > Fine with me. +1 on this. Let me check other patches first. If there is no need for another round of posting of this series, the addition of the disclaimers can come in a separate patch. Wei. > > > Juergen >
diff --git a/tools/libs/store/include/xenstore.h b/tools/libs/store/include/xenstore.h index cbc7206a0f..158e69ef83 100644 --- a/tools/libs/store/include/xenstore.h +++ b/tools/libs/store/include/xenstore.h @@ -60,15 +60,12 @@ typedef uint32_t xs_transaction_t; /* Open a connection to the xs daemon. * Attempts to make a connection over the socket interface, * and if it fails, then over the xenbus interface. - * Mode 0 specifies read-write access, XS_OPEN_READONLY for - * read-only access. * * * Connections made with xs_open(0) (which might be shared page or * socket based) are only guaranteed to work in the parent after * fork. * * xs_daemon_open*() and xs_domain_open() are deprecated synonyms * for xs_open(0). - * * XS_OPEN_READONLY has no bearing on any of this. * * Returns a handle or NULL. */ @@ -83,11 +80,6 @@ void xs_close(struct xs_handle *xsh /* NULL ok */); */ struct xs_handle *xs_daemon_open(void); struct xs_handle *xs_domain_open(void); - -/* Connect to the xs daemon (readonly for non-root clients). - * Returns a handle or NULL. - * Deprecated, please use xs_open(XS_OPEN_READONLY) instead - */ struct xs_handle *xs_daemon_open_readonly(void); /* Close the connection to the xs daemon. diff --git a/tools/libs/store/xs.c b/tools/libs/store/xs.c index 320734416f..4ac73ec317 100644 --- a/tools/libs/store/xs.c +++ b/tools/libs/store/xs.c @@ -302,7 +302,7 @@ struct xs_handle *xs_daemon_open(void) struct xs_handle *xs_daemon_open_readonly(void) { - return xs_open(XS_OPEN_READONLY); + return xs_open(0); } struct xs_handle *xs_domain_open(void) @@ -314,10 +314,7 @@ struct xs_handle *xs_open(unsigned long flags) { struct xs_handle *xsh = NULL; - if (flags & XS_OPEN_READONLY) - xsh = get_handle(xs_daemon_socket_ro()); - else - xsh = get_handle(xs_daemon_socket()); + xsh = get_handle(xs_daemon_socket()); if (!xsh) xsh = get_handle(xs_domain_dev());
Today it is possible to open the connection in read-only mode via xs_daemon_open_readonly(). This is working only with Xenstore running as a daemon in the same domain as the user. Additionally it doesn't add any security as accessing the socket used for that functionality requires the same privileges as the socket used for full Xenstore access. So just drop the read-only semantics in all cases, leaving the interface existing only for compatibility reasons. This in turn requires to just ignore the XS_OPEN_READONLY flag. Signed-off-by: Juergen Gross <jgross@suse.com> --- tools/libs/store/include/xenstore.h | 8 -------- tools/libs/store/xs.c | 7 ++----- 2 files changed, 2 insertions(+), 13 deletions(-)