| Message ID | 20230724110247.10520-23-jgross@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | tools/xenstore: drop TDB | expand |
Hi, On 24/07/2023 12:02, Juergen Gross wrote: > diff --git a/tools/xenstore/xenstored_watch.c b/tools/xenstore/xenstored_watch.c > index 86cf8322b4..2662a3fa49 100644 > --- a/tools/xenstore/xenstored_watch.c > +++ b/tools/xenstore/xenstored_watch.c > @@ -166,19 +166,12 @@ static int destroy_watch(void *_watch) > static int check_watch_path(struct connection *conn, const void *ctx, > const char **path, bool *relative) > { > - /* Check if valid event. */ > - if (strstarts(*path, "@")) { > - *relative = false; > - if (strlen(*path) > XENSTORE_REL_PATH_MAX) > - goto inval; I can't find an exact matching check in is_valid_nodename(). The call also seems to put more restriction on '@' node. Can you clarify? > - } else { > - *relative = !strstarts(*path, "/"); > - *path = canonicalize(conn, ctx, *path); > - if (!*path) > - return errno; > - if (!is_valid_nodename(conn, *path)) > - goto inval; > - } > + *relative = !strstarts(*path, "/") && !strstarts(*path, "@"); > + *path = canonicalize(conn, ctx, *path); > + if (!*path) > + return errno; > + if (!is_valid_nodename(conn, *path, true)) > + goto inval; > > return 0; > Cheers,
On 03.08.23 23:36, Julien Grall wrote: > Hi, > > On 24/07/2023 12:02, Juergen Gross wrote: >> diff --git a/tools/xenstore/xenstored_watch.c b/tools/xenstore/xenstored_watch.c >> index 86cf8322b4..2662a3fa49 100644 >> --- a/tools/xenstore/xenstored_watch.c >> +++ b/tools/xenstore/xenstored_watch.c >> @@ -166,19 +166,12 @@ static int destroy_watch(void *_watch) >> static int check_watch_path(struct connection *conn, const void *ctx, >> const char **path, bool *relative) >> { >> - /* Check if valid event. */ >> - if (strstarts(*path, "@")) { >> - *relative = false; >> - if (strlen(*path) > XENSTORE_REL_PATH_MAX) >> - goto inval; > > I can't find an exact matching check in is_valid_nodename(). The call also seems > to put more restriction on '@' node. Can you clarify? The call of domain_max_chk() in is_valid_nodename() will check the length of the node name (at least for unprivileged callers, which is the important case). The additional restrictions for special nodes are: - they can't end with "/" - they can't contain "//" - they can't contain characters other than the ones allowed for normal nodes None of those restrictions are problematic. I can add something to the commit message if you want. Juergen
Hi, On 04/08/2023 10:17, Juergen Gross wrote: > On 03.08.23 23:36, Julien Grall wrote: >> Hi, >> >> On 24/07/2023 12:02, Juergen Gross wrote: >>> diff --git a/tools/xenstore/xenstored_watch.c >>> b/tools/xenstore/xenstored_watch.c >>> index 86cf8322b4..2662a3fa49 100644 >>> --- a/tools/xenstore/xenstored_watch.c >>> +++ b/tools/xenstore/xenstored_watch.c >>> @@ -166,19 +166,12 @@ static int destroy_watch(void *_watch) >>> static int check_watch_path(struct connection *conn, const void *ctx, >>> const char **path, bool *relative) >>> { >>> - /* Check if valid event. */ >>> - if (strstarts(*path, "@")) { >>> - *relative = false; >>> - if (strlen(*path) > XENSTORE_REL_PATH_MAX) >>> - goto inval; >> >> I can't find an exact matching check in is_valid_nodename(). The call >> also seems to put more restriction on '@' node. Can you clarify? > > The call of domain_max_chk() in is_valid_nodename() will check the length > of the node name (at least for unprivileged callers, which is the important > case). Right, but from my understanding, this may not check against XENSTORE_REL_PATH_MAX but whatever limit the user set. This is a change of behavior that you ought to be explained. > > The additional restrictions for special nodes are: > > - they can't end with "/" > - they can't contain "//" > - they can't contain characters other than the ones allowed for normal > nodes > > None of those restrictions are problematic. I can add something to the > commit message if you want. Yes please. Cheers,
On 04.08.23 11:21, Julien Grall wrote: > Hi, > > On 04/08/2023 10:17, Juergen Gross wrote: >> On 03.08.23 23:36, Julien Grall wrote: >>> Hi, >>> >>> On 24/07/2023 12:02, Juergen Gross wrote: >>>> diff --git a/tools/xenstore/xenstored_watch.c >>>> b/tools/xenstore/xenstored_watch.c >>>> index 86cf8322b4..2662a3fa49 100644 >>>> --- a/tools/xenstore/xenstored_watch.c >>>> +++ b/tools/xenstore/xenstored_watch.c >>>> @@ -166,19 +166,12 @@ static int destroy_watch(void *_watch) >>>> static int check_watch_path(struct connection *conn, const void *ctx, >>>> const char **path, bool *relative) >>>> { >>>> - /* Check if valid event. */ >>>> - if (strstarts(*path, "@")) { >>>> - *relative = false; >>>> - if (strlen(*path) > XENSTORE_REL_PATH_MAX) >>>> - goto inval; >>> >>> I can't find an exact matching check in is_valid_nodename(). The call also >>> seems to put more restriction on '@' node. Can you clarify? >> >> The call of domain_max_chk() in is_valid_nodename() will check the length >> of the node name (at least for unprivileged callers, which is the important >> case). > > Right, but from my understanding, this may not check against > XENSTORE_REL_PATH_MAX but whatever limit the user set. Yes, and that's what should be tested, right? I don't see why special nodes should not adhere to the same limits as other nodes. In case an unprivileged user should have access to special nodes, the limits shouldn't hinder the user to access those nodes (setting a limit below 15 would be ridiculous anyway, and that is the length of longest special node name today). > This is a change of behavior that you ought to be explained. > >> >> The additional restrictions for special nodes are: >> >> - they can't end with "/" >> - they can't contain "//" >> - they can't contain characters other than the ones allowed for normal nodes >> >> None of those restrictions are problematic. I can add something to the >> commit message if you want. > > Yes please. Juergen
On 04/08/2023 10:34, Juergen Gross wrote: > On 04.08.23 11:21, Julien Grall wrote: >> Hi, >> >> On 04/08/2023 10:17, Juergen Gross wrote: >>> On 03.08.23 23:36, Julien Grall wrote: >>>> Hi, >>>> >>>> On 24/07/2023 12:02, Juergen Gross wrote: >>>>> diff --git a/tools/xenstore/xenstored_watch.c >>>>> b/tools/xenstore/xenstored_watch.c >>>>> index 86cf8322b4..2662a3fa49 100644 >>>>> --- a/tools/xenstore/xenstored_watch.c >>>>> +++ b/tools/xenstore/xenstored_watch.c >>>>> @@ -166,19 +166,12 @@ static int destroy_watch(void *_watch) >>>>> static int check_watch_path(struct connection *conn, const void >>>>> *ctx, >>>>> const char **path, bool *relative) >>>>> { >>>>> - /* Check if valid event. */ >>>>> - if (strstarts(*path, "@")) { >>>>> - *relative = false; >>>>> - if (strlen(*path) > XENSTORE_REL_PATH_MAX) >>>>> - goto inval; >>>> >>>> I can't find an exact matching check in is_valid_nodename(). The >>>> call also seems to put more restriction on '@' node. Can you clarify? >>> >>> The call of domain_max_chk() in is_valid_nodename() will check the >>> length >>> of the node name (at least for unprivileged callers, which is the >>> important >>> case). >> >> Right, but from my understanding, this may not check against >> XENSTORE_REL_PATH_MAX but whatever limit the user set. > > Yes, and that's what should be tested, right? I don't see why special nodes > should not adhere to the same limits as other nodes. In case an > unprivileged > user should have access to special nodes, the limits shouldn't hinder the > user to access those nodes (setting a limit below 15 would be ridiculous > anyway, and that is the length of longest special node name today). I don't mind you want to test against a different value. My point is more that you didn't mention that the limit will be changed. Cheers,
On 04.08.23 11:44, Julien Grall wrote: > > > On 04/08/2023 10:34, Juergen Gross wrote: >> On 04.08.23 11:21, Julien Grall wrote: >>> Hi, >>> >>> On 04/08/2023 10:17, Juergen Gross wrote: >>>> On 03.08.23 23:36, Julien Grall wrote: >>>>> Hi, >>>>> >>>>> On 24/07/2023 12:02, Juergen Gross wrote: >>>>>> diff --git a/tools/xenstore/xenstored_watch.c >>>>>> b/tools/xenstore/xenstored_watch.c >>>>>> index 86cf8322b4..2662a3fa49 100644 >>>>>> --- a/tools/xenstore/xenstored_watch.c >>>>>> +++ b/tools/xenstore/xenstored_watch.c >>>>>> @@ -166,19 +166,12 @@ static int destroy_watch(void *_watch) >>>>>> static int check_watch_path(struct connection *conn, const void *ctx, >>>>>> const char **path, bool *relative) >>>>>> { >>>>>> - /* Check if valid event. */ >>>>>> - if (strstarts(*path, "@")) { >>>>>> - *relative = false; >>>>>> - if (strlen(*path) > XENSTORE_REL_PATH_MAX) >>>>>> - goto inval; >>>>> >>>>> I can't find an exact matching check in is_valid_nodename(). The call also >>>>> seems to put more restriction on '@' node. Can you clarify? >>>> >>>> The call of domain_max_chk() in is_valid_nodename() will check the length >>>> of the node name (at least for unprivileged callers, which is the important >>>> case). >>> >>> Right, but from my understanding, this may not check against >>> XENSTORE_REL_PATH_MAX but whatever limit the user set. >> >> Yes, and that's what should be tested, right? I don't see why special nodes >> should not adhere to the same limits as other nodes. In case an unprivileged >> user should have access to special nodes, the limits shouldn't hinder the >> user to access those nodes (setting a limit below 15 would be ridiculous >> anyway, and that is the length of longest special node name today). > I don't mind you want to test against a different value. My point is more that > you didn't mention that the limit will be changed. Will mention it. Juergen
diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c index 102be92a43..ea5a1a9cce 100644 --- a/tools/xenstore/xenstored_core.c +++ b/tools/xenstore/xenstored_core.c @@ -1219,13 +1219,14 @@ static bool valid_chars(const char *node) "0123456789-/_@") == strlen(node)); } -bool is_valid_nodename(const struct connection *conn, const char *node) +bool is_valid_nodename(const struct connection *conn, const char *node, + bool allow_special) { int local_off = 0; unsigned int domid; - /* Must start in /. */ - if (!strstarts(node, "/")) + /* Must start in / or - if special nodes are allowed - in @. */ + if (!strstarts(node, "/") && (!allow_special || !strstarts(node, "@"))) return false; /* Cannot end in / (unless it's just "/"). */ @@ -1294,7 +1295,8 @@ static struct node *get_node_canonicalized(struct connection *conn, const void *ctx, const char *name, const char **canonical_name, - unsigned int perm) + unsigned int perm, + bool allow_special) { const char *tmp_name; @@ -1303,33 +1305,20 @@ static struct node *get_node_canonicalized(struct connection *conn, *canonical_name = canonicalize(conn, ctx, name); if (!*canonical_name) return NULL; - if (!is_valid_nodename(conn, *canonical_name)) { + if (!is_valid_nodename(conn, *canonical_name, allow_special)) { errno = EINVAL; return NULL; } return get_node(conn, ctx, *canonical_name, perm); } -static struct node *get_spec_node(struct connection *conn, const void *ctx, - const char *name, const char **canonical_name, - unsigned int perm) -{ - if (name[0] == '@') { - if (canonical_name) - *canonical_name = name; - return get_node(conn, ctx, name, perm); - } - - return get_node_canonicalized(conn, ctx, name, canonical_name, perm); -} - static int send_directory(const void *ctx, struct connection *conn, struct buffered_data *in) { struct node *node; node = get_node_canonicalized(conn, ctx, onearg(in), NULL, - XS_PERM_READ); + XS_PERM_READ, false); if (!node) return errno; @@ -1351,7 +1340,7 @@ static int send_directory_part(const void *ctx, struct connection *conn, /* First arg is node name. */ node = get_node_canonicalized(conn, ctx, in->buffer, NULL, - XS_PERM_READ); + XS_PERM_READ, false); if (!node) return errno; @@ -1401,7 +1390,7 @@ static int do_read(const void *ctx, struct connection *conn, struct node *node; node = get_node_canonicalized(conn, ctx, onearg(in), NULL, - XS_PERM_READ); + XS_PERM_READ, false); if (!node) return errno; @@ -1615,7 +1604,8 @@ static int do_write(const void *ctx, struct connection *conn, offset = strlen(vec[0]) + 1; datalen = in->used - offset; - node = get_node_canonicalized(conn, ctx, vec[0], &name, XS_PERM_WRITE); + node = get_node_canonicalized(conn, ctx, vec[0], &name, XS_PERM_WRITE, + false); if (!node) { /* No permissions, invalid input? */ if (errno != ENOENT) @@ -1644,7 +1634,7 @@ static int do_mkdir(const void *ctx, struct connection *conn, const char *name; node = get_node_canonicalized(conn, ctx, onearg(in), &name, - XS_PERM_WRITE); + XS_PERM_WRITE, false); /* If it already exists, fine. */ if (!node) { @@ -1774,7 +1764,7 @@ static int do_rm(const void *ctx, struct connection *conn, char *parentname; node = get_node_canonicalized(conn, ctx, onearg(in), &name, - XS_PERM_WRITE); + XS_PERM_WRITE, false); if (!node) { /* Didn't exist already? Fine, if parent exists. */ if (errno == ENOENT) { @@ -1816,7 +1806,8 @@ static int do_get_perms(const void *ctx, struct connection *conn, unsigned int len; struct node_perms perms; - node = get_spec_node(conn, ctx, onearg(in), NULL, XS_PERM_READ); + node = get_node_canonicalized(conn, ctx, onearg(in), NULL, XS_PERM_READ, + true); if (!node) return errno; @@ -1860,8 +1851,8 @@ static int do_set_perms(const void *ctx, struct connection *conn, return ENOENT; /* We must own node to do this (tools can do this too). */ - node = get_spec_node(conn, ctx, in->buffer, &name, - XS_PERM_WRITE | XS_PERM_OWNER); + node = get_node_canonicalized(conn, ctx, in->buffer, &name, + XS_PERM_WRITE | XS_PERM_OWNER, true); if (!node) return errno; diff --git a/tools/xenstore/xenstored_core.h b/tools/xenstore/xenstored_core.h index 65782c559d..f3a83efce8 100644 --- a/tools/xenstore/xenstored_core.h +++ b/tools/xenstore/xenstored_core.h @@ -295,7 +295,8 @@ void check_store(void); void corrupt(struct connection *conn, const char *fmt, ...); /* Is this a valid node name? */ -bool is_valid_nodename(const struct connection *conn, const char *node); +bool is_valid_nodename(const struct connection *conn, const char *node, + bool allow_special); /* Get name of parent node. */ char *get_parent(const void *ctx, const char *node); diff --git a/tools/xenstore/xenstored_watch.c b/tools/xenstore/xenstored_watch.c index 86cf8322b4..2662a3fa49 100644 --- a/tools/xenstore/xenstored_watch.c +++ b/tools/xenstore/xenstored_watch.c @@ -166,19 +166,12 @@ static int destroy_watch(void *_watch) static int check_watch_path(struct connection *conn, const void *ctx, const char **path, bool *relative) { - /* Check if valid event. */ - if (strstarts(*path, "@")) { - *relative = false; - if (strlen(*path) > XENSTORE_REL_PATH_MAX) - goto inval; - } else { - *relative = !strstarts(*path, "/"); - *path = canonicalize(conn, ctx, *path); - if (!*path) - return errno; - if (!is_valid_nodename(conn, *path)) - goto inval; - } + *relative = !strstarts(*path, "/") && !strstarts(*path, "@"); + *path = canonicalize(conn, ctx, *path); + if (!*path) + return errno; + if (!is_valid_nodename(conn, *path, true)) + goto inval; return 0;
Add a "allow_special" parameter to get_node_canonicalized() allowing to merge get_spec_node() into get_node_canonicalized(). Add the same parameter to is_valid_nodename(), as this will simplify check_watch_path(). This is done in preparation to introducing a get_node() variant returning a pointer to const struct node. Signed-off-by: Juergen Gross <jgross@suse.com> --- V3: - new patch --- tools/xenstore/xenstored_core.c | 45 +++++++++++++------------------- tools/xenstore/xenstored_core.h | 3 ++- tools/xenstore/xenstored_watch.c | 19 +++++--------- 3 files changed, 26 insertions(+), 41 deletions(-)