| Message ID | 149399503288.29022.13904385823492355197.stgit@bahia.lan |
|---|---|
| State | New |
| Headers | show |
On 05/05/2017 09:37 AM, Greg Kurz wrote: > When using the mapped-file security mode, the creds of a path /foo/bar > are stored in the /foo/.virtfs_metadata/bar file. This is okay for all > paths unless they end with '.' or '..', because we cannot create the > corresponding file in the metadata directory. > > This patch ensures that '.' and '..' are resolved in all paths. > > The core code only passes path elements (no '/') to the backend, with > the notable exception of the '/' path, which refers to the virtfs root. > This patch preserve the current behavior of converting it to '.' so s/preserve/preserves/ > that it can be passed to "*at()" syscalls ('/' would mean the host root). > > Signed-off-by: Greg Kurz <groug@kaod.org> > --- > hw/9pfs/9p-local.c | 30 +++++++++++++++++++++++------- > 1 file changed, 23 insertions(+), 7 deletions(-) > > diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c > index f3ebca4f7a56..92262f3c3e37 100644 > --- a/hw/9pfs/9p-local.c > +++ b/hw/9pfs/9p-local.c > @@ -1097,14 +1097,30 @@ static int local_name_to_path(FsContext *ctx, V9fsPath *dir_path, > const char *name, V9fsPath *target) > { > if (dir_path) { > - v9fs_path_sprintf(target, "%s/%s", dir_path->data, name); > - } else if (strcmp(name, "/")) { > - v9fs_path_sprintf(target, "%s", name); > + if (!strcmp(name, ".")) { > + /* "." relative to "foo/bar" is "foo/bar" */ > + v9fs_path_copy(target, dir_path); > + } else if (!strcmp(name, "..")) { > + if (!strcmp(dir_path->data, ".")) { > + /* ".." relative to the root is "." */ > + v9fs_path_sprintf(target, "."); > + } else { > + char *tmp = g_path_get_dirname(dir_path->data); > + /* ".." relative to "foo/bar" is equivalent to "foo" */ True only if bar is not a symlink to some other directory. What guarantees do you have that you are not going to be inadvertently skipping a traversal through symlinks and thereby picking the wrong location for '..'? > + v9fs_path_sprintf(target, "%s", tmp); > + g_free(tmp); > + } > + } else { > + assert(!strchr(name, '/')); > + v9fs_path_sprintf(target, "%s/%s", dir_path->data, name); > + } > + } else if (!strcmp(name, "/") || !strcmp(name, ".") || > + !strcmp(name, "..")) { > + /* This is the root fid */ > + v9fs_path_sprintf(target, "."); > } else { > - /* We want the path of the export root to be relative, otherwise > - * "*at()" syscalls would treat it as "/" in the host. > - */ > - v9fs_path_sprintf(target, "%s", "."); > + assert(!strchr(name, '/')); > + v9fs_path_sprintf(target, "./%s", name); > } > return 0; > } > >
On Fri, 5 May 2017 11:59:15 -0500 Eric Blake <eblake@redhat.com> wrote: > On 05/05/2017 09:37 AM, Greg Kurz wrote: > > When using the mapped-file security mode, the creds of a path /foo/bar > > are stored in the /foo/.virtfs_metadata/bar file. This is okay for all > > paths unless they end with '.' or '..', because we cannot create the > > corresponding file in the metadata directory. > > > > This patch ensures that '.' and '..' are resolved in all paths. > > > > The core code only passes path elements (no '/') to the backend, with > > the notable exception of the '/' path, which refers to the virtfs root. > > This patch preserve the current behavior of converting it to '.' so > > s/preserve/preserves/ > I'll fix this. > > that it can be passed to "*at()" syscalls ('/' would mean the host root). > > > > Signed-off-by: Greg Kurz <groug@kaod.org> > > --- > > hw/9pfs/9p-local.c | 30 +++++++++++++++++++++++------- > > 1 file changed, 23 insertions(+), 7 deletions(-) > > > > diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c > > index f3ebca4f7a56..92262f3c3e37 100644 > > --- a/hw/9pfs/9p-local.c > > +++ b/hw/9pfs/9p-local.c > > @@ -1097,14 +1097,30 @@ static int local_name_to_path(FsContext *ctx, V9fsPath *dir_path, > > const char *name, V9fsPath *target) > > { > > if (dir_path) { > > - v9fs_path_sprintf(target, "%s/%s", dir_path->data, name); > > - } else if (strcmp(name, "/")) { > > - v9fs_path_sprintf(target, "%s", name); > > + if (!strcmp(name, ".")) { > > + /* "." relative to "foo/bar" is "foo/bar" */ > > + v9fs_path_copy(target, dir_path); > > + } else if (!strcmp(name, "..")) { > > + if (!strcmp(dir_path->data, ".")) { > > + /* ".." relative to the root is "." */ > > + v9fs_path_sprintf(target, "."); > > + } else { > > + char *tmp = g_path_get_dirname(dir_path->data); > > + /* ".." relative to "foo/bar" is equivalent to "foo" */ > > True only if bar is not a symlink to some other directory. What > guarantees do you have that you are not going to be inadvertently > skipping a traversal through symlinks and thereby picking the wrong > location for '..'? > My understanding is that symlinks are supposed to be resolved by the client, and we shouldn't follow them in the server. > > + v9fs_path_sprintf(target, "%s", tmp); > > + g_free(tmp); > > + } > > + } else { > > + assert(!strchr(name, '/')); > > + v9fs_path_sprintf(target, "%s/%s", dir_path->data, name); > > + } > > + } else if (!strcmp(name, "/") || !strcmp(name, ".") || > > + !strcmp(name, "..")) { > > + /* This is the root fid */ > > + v9fs_path_sprintf(target, "."); > > } else { > > - /* We want the path of the export root to be relative, otherwise > > - * "*at()" syscalls would treat it as "/" in the host. > > - */ > > - v9fs_path_sprintf(target, "%s", "."); > > + assert(!strchr(name, '/')); > > + v9fs_path_sprintf(target, "./%s", name); > > } > > return 0; > > } > > > > >
On Tue, 9 May 2017 11:12:58 +0200 Greg Kurz <groug@kaod.org> wrote: > On Fri, 5 May 2017 11:59:15 -0500 > Eric Blake <eblake@redhat.com> wrote: > > > On 05/05/2017 09:37 AM, Greg Kurz wrote: > > > When using the mapped-file security mode, the creds of a path /foo/bar > > > are stored in the /foo/.virtfs_metadata/bar file. This is okay for all > > > paths unless they end with '.' or '..', because we cannot create the > > > corresponding file in the metadata directory. > > > > > > This patch ensures that '.' and '..' are resolved in all paths. > > > > > > The core code only passes path elements (no '/') to the backend, with > > > the notable exception of the '/' path, which refers to the virtfs root. > > > This patch preserve the current behavior of converting it to '.' so [...] > > > + } else if (!strcmp(name, "..")) { > > > + if (!strcmp(dir_path->data, ".")) { > > > + /* ".." relative to the root is "." */ > > > + v9fs_path_sprintf(target, "."); > > > + } else { > > > + char *tmp = g_path_get_dirname(dir_path->data); > > > + /* ".." relative to "foo/bar" is equivalent to "foo" */ > > > > True only if bar is not a symlink to some other directory. What > > guarantees do you have that you are not going to be inadvertently > > skipping a traversal through symlinks and thereby picking the wrong > > location for '..'? > > > > My understanding is that symlinks are supposed to be resolved by the client, > and we shouldn't follow them in the server. > Eric, Do you have any comment further comment or can I go on with this change ? Cheers, -- Greg
On 05/18/2017 03:41 AM, Greg Kurz wrote: > [...] >>>> + } else if (!strcmp(name, "..")) { >>>> + if (!strcmp(dir_path->data, ".")) { >>>> + /* ".." relative to the root is "." */ >>>> + v9fs_path_sprintf(target, "."); >>>> + } else { >>>> + char *tmp = g_path_get_dirname(dir_path->data); >>>> + /* ".." relative to "foo/bar" is equivalent to "foo" */ >>> >>> True only if bar is not a symlink to some other directory. What >>> guarantees do you have that you are not going to be inadvertently >>> skipping a traversal through symlinks and thereby picking the wrong >>> location for '..'? >>> >> >> My understanding is that symlinks are supposed to be resolved by the client, >> and we shouldn't follow them in the server. >> > > Eric, > > Do you have any comment further comment or can I go on with this change ? I'd update the comment to mention that the equivalency between "foo/bar/.." and "foo" is _because_ the client has already resolved symlinks, and then I think you're good to go.
diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c index f3ebca4f7a56..92262f3c3e37 100644 --- a/hw/9pfs/9p-local.c +++ b/hw/9pfs/9p-local.c @@ -1097,14 +1097,30 @@ static int local_name_to_path(FsContext *ctx, V9fsPath *dir_path, const char *name, V9fsPath *target) { if (dir_path) { - v9fs_path_sprintf(target, "%s/%s", dir_path->data, name); - } else if (strcmp(name, "/")) { - v9fs_path_sprintf(target, "%s", name); + if (!strcmp(name, ".")) { + /* "." relative to "foo/bar" is "foo/bar" */ + v9fs_path_copy(target, dir_path); + } else if (!strcmp(name, "..")) { + if (!strcmp(dir_path->data, ".")) { + /* ".." relative to the root is "." */ + v9fs_path_sprintf(target, "."); + } else { + char *tmp = g_path_get_dirname(dir_path->data); + /* ".." relative to "foo/bar" is equivalent to "foo" */ + v9fs_path_sprintf(target, "%s", tmp); + g_free(tmp); + } + } else { + assert(!strchr(name, '/')); + v9fs_path_sprintf(target, "%s/%s", dir_path->data, name); + } + } else if (!strcmp(name, "/") || !strcmp(name, ".") || + !strcmp(name, "..")) { + /* This is the root fid */ + v9fs_path_sprintf(target, "."); } else { - /* We want the path of the export root to be relative, otherwise - * "*at()" syscalls would treat it as "/" in the host. - */ - v9fs_path_sprintf(target, "%s", "."); + assert(!strchr(name, '/')); + v9fs_path_sprintf(target, "./%s", name); } return 0; }
When using the mapped-file security mode, the creds of a path /foo/bar are stored in the /foo/.virtfs_metadata/bar file. This is okay for all paths unless they end with '.' or '..', because we cannot create the corresponding file in the metadata directory. This patch ensures that '.' and '..' are resolved in all paths. The core code only passes path elements (no '/') to the backend, with the notable exception of the '/' path, which refers to the virtfs root. This patch preserve the current behavior of converting it to '.' so that it can be passed to "*at()" syscalls ('/' would mean the host root). Signed-off-by: Greg Kurz <groug@kaod.org> --- hw/9pfs/9p-local.c | 30 +++++++++++++++++++++++------- 1 file changed, 23 insertions(+), 7 deletions(-)