| Message ID | 147222405454.18925.2135759955496138955.stgit@bahia.lan |
|---|---|
| State | New |
| Headers | show |
On 08/26/2016 10:07 AM, Greg Kurz wrote: > A buggy guest using the 9p2000.u protocol can issue a create request and > pass an empty string as the extension argument. This causes QEMU to crash > in the case of a hard link or a special file, and leads to undefined > behavior, depending on the backend, in the case of a symbolic link. > > This patch causes the request to fail with EINVAL in these scenarios. > > Signed-off-by: Greg Kurz <groug@kaod.org> > --- > hw/9pfs/9p.c | 21 +++++++++++++++++++-- > 1 file changed, 19 insertions(+), 2 deletions(-) > > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c > index 7b1dfe4e47cb..dc65c3125006 100644 > --- a/hw/9pfs/9p.c > +++ b/hw/9pfs/9p.c > @@ -2150,6 +2150,11 @@ static void v9fs_create(void *opaque) > } > fidp->fid_type = P9_FID_DIR; > } else if (perm & P9_STAT_MODE_SYMLINK) { > + if (extension.data == NULL) { > + err = -EINVAL; > + goto out; > + } POSIX specifically requires implementations to support creating a symlink whose target is the empty string. Linux doesn't [yet] permit it, but BSD does. On systems where creating such a symlink is legal, POSIX requires that such a symlink either be treated as "." if dereferenced, or be treated as ENOENT on attempt to dereference. But since such links can be created, readlink() should be able to read them without error. I would argue that we should NOT forbid empty symlinks on creation (but pass back any error from the underlying host OS); but instead check that dereferencing such a symlink behaves sanely if it was created. Meanwhile, a client should not be relying on the behavior (since Linux disobeys POSIX, portable clients should already be avoiding empty symlinks). http://austingroupbugs.net/view.php?id=649 > @@ -2161,8 +2166,15 @@ static void v9fs_create(void *opaque) > } > v9fs_path_copy(&fidp->path, &path); > } else if (perm & P9_STAT_MODE_LINK) { > - int32_t ofid = atoi(extension.data); > - V9fsFidState *ofidp = get_fid(pdu, ofid); > + V9fsFidState *ofidp; > + > + if (extension.data == NULL) { > + err = -EINVAL; > + goto out; > + } Rejecting an empty destination on hard link or device creation, however, is indeed appropriate.
On Fri, Aug 26, 2016 at 02:00:37PM -0500, Eric Blake wrote: > On 08/26/2016 10:07 AM, Greg Kurz wrote: > > A buggy guest using the 9p2000.u protocol can issue a create request and > > pass an empty string as the extension argument. This causes QEMU to crash > > in the case of a hard link or a special file, and leads to undefined > > behavior, depending on the backend, in the case of a symbolic link. > > > > This patch causes the request to fail with EINVAL in these scenarios. > > > > Signed-off-by: Greg Kurz <groug@kaod.org> > > --- > > hw/9pfs/9p.c | 21 +++++++++++++++++++-- > > 1 file changed, 19 insertions(+), 2 deletions(-) > > > > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c > > index 7b1dfe4e47cb..dc65c3125006 100644 > > --- a/hw/9pfs/9p.c > > +++ b/hw/9pfs/9p.c > > @@ -2150,6 +2150,11 @@ static void v9fs_create(void *opaque) > > } > > fidp->fid_type = P9_FID_DIR; > > } else if (perm & P9_STAT_MODE_SYMLINK) { > > + if (extension.data == NULL) { > > + err = -EINVAL; > > + goto out; > > + } > > POSIX specifically requires implementations to support creating a > symlink whose target is the empty string. Linux doesn't [yet] permit > it, but BSD does. On systems where creating such a symlink is legal, > POSIX requires that such a symlink either be treated as "." if > dereferenced, or be treated as ENOENT on attempt to dereference. But > since such links can be created, readlink() should be able to read them > without error. > > I would argue that we should NOT forbid empty symlinks on creation (but > pass back any error from the underlying host OS); but instead check that > dereferencing such a symlink behaves sanely if it was created. > Meanwhile, a client should not be relying on the behavior (since Linux > disobeys POSIX, portable clients should already be avoiding empty symlinks). > > http://austingroupbugs.net/view.php?id=649 Given 9p is only supported on Linux hosts, I think this patch's approach is OK for now, and it's certainly much simpler than worrying about the fallout of allowing empty names. A TODO that documents your suggestions, and including the considerations in the comment would be a good idea. > > @@ -2161,8 +2166,15 @@ static void v9fs_create(void *opaque) > > } > > v9fs_path_copy(&fidp->path, &path); > > } else if (perm & P9_STAT_MODE_LINK) { > > - int32_t ofid = atoi(extension.data); > > - V9fsFidState *ofidp = get_fid(pdu, ofid); > > + V9fsFidState *ofidp; > > + > > + if (extension.data == NULL) { > > + err = -EINVAL; > > + goto out; > > + } > > Rejecting an empty destination on hard link or device creation, however, > is indeed appropriate. > > -- > Eric Blake eblake redhat com +1-919-301-3266 > Libvirt virtualization library http://libvirt.org >
On Fri, 26 Aug 2016 14:00:37 -0500 Eric Blake <eblake@redhat.com> wrote: > On 08/26/2016 10:07 AM, Greg Kurz wrote: > > A buggy guest using the 9p2000.u protocol can issue a create request and > > pass an empty string as the extension argument. This causes QEMU to crash > > in the case of a hard link or a special file, and leads to undefined > > behavior, depending on the backend, in the case of a symbolic link. > > > > This patch causes the request to fail with EINVAL in these scenarios. > > > > Signed-off-by: Greg Kurz <groug@kaod.org> > > --- Wait... empty strings coming from pdu_unmarshal() never have data == NULL so this whole patch is pointless :) and BTW, only the symlink case is about file names. > > hw/9pfs/9p.c | 21 +++++++++++++++++++-- > > 1 file changed, 19 insertions(+), 2 deletions(-) > > > > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c > > index 7b1dfe4e47cb..dc65c3125006 100644 > > --- a/hw/9pfs/9p.c > > +++ b/hw/9pfs/9p.c > > @@ -2150,6 +2150,11 @@ static void v9fs_create(void *opaque) > > } > > fidp->fid_type = P9_FID_DIR; > > } else if (perm & P9_STAT_MODE_SYMLINK) { > > + if (extension.data == NULL) { > > + err = -EINVAL; > > + goto out; > > + } > I realize that this part belongs to patch 1 actually: it is the implementation of symbolic links that comes with 9P2000.u (different from the TSYMLINK request in 9P2000.L). In which case, the hunk would have been: + if (name_is_illegal(extension.data)) { + err = -EINVAL; + goto out; + } > POSIX specifically requires implementations to support creating a > symlink whose target is the empty string. Linux doesn't [yet] permit > it, but BSD does. On systems where creating such a symlink is legal, > POSIX requires that such a symlink either be treated as "." if > dereferenced, or be treated as ENOENT on attempt to dereference. But > since such links can be created, readlink() should be able to read them > without error. > > I would argue that we should NOT forbid empty symlinks on creation (but > pass back any error from the underlying host OS); but instead check that > dereferencing such a symlink behaves sanely if it was created. > Meanwhile, a client should not be relying on the behavior (since Linux > disobeys POSIX, portable clients should already be avoiding empty symlinks). > > http://austingroupbugs.net/view.php?id=649 > Indeed, maybe we should let the backend decide if it allows symlink with an empty target, since the target name is simply "stored" somewhere and not used to create a new path. In which case, we should do the same with v9fs_symlink(). And we would have two exceptions to the name_is_illegal() helper, because we still want to avoid '/' in file names... On the other hand, we only support linux hosts where the call to symlink() will fail with ENOENT and guests using the official linux kernel 9p client never send an empty target... For the sake of simplicity, I'd rather have the target names to follow the same rules as other file names, and return ENOENT directly (the link you provide states it is a valid option). Peter, Since you suggested to do explicit error checking on empty file names, do you have an opinion on the case of symlinks with an empty target ? > > @@ -2161,8 +2166,15 @@ static void v9fs_create(void *opaque) > > } > > v9fs_path_copy(&fidp->path, &path); > > } else if (perm & P9_STAT_MODE_LINK) { > > - int32_t ofid = atoi(extension.data); > > - V9fsFidState *ofidp = get_fid(pdu, ofid); > > + V9fsFidState *ofidp; > > + > > + if (extension.data == NULL) { > > + err = -EINVAL; > > + goto out; > > + } > > Rejecting an empty destination on hard link or device creation, however, > is indeed appropriate. > In the case of hard links, extension.data is a FID, not a file name. In the case of device creation, extension.data is "type major minor", not a file name again.
On Sun, 28 Aug 2016 19:21:25 +0200 Greg Kurz <groug@kaod.org> wrote: > On Fri, 26 Aug 2016 14:00:37 -0500 > Eric Blake <eblake@redhat.com> wrote: > > > On 08/26/2016 10:07 AM, Greg Kurz wrote: > > > A buggy guest using the 9p2000.u protocol can issue a create request and > > > pass an empty string as the extension argument. This causes QEMU to crash > > > in the case of a hard link or a special file, and leads to undefined > > > behavior, depending on the backend, in the case of a symbolic link. > > > > > > This patch causes the request to fail with EINVAL in these scenarios. > > > > > > Signed-off-by: Greg Kurz <groug@kaod.org> > > > --- > > Wait... empty strings coming from pdu_unmarshal() never have data == NULL > so this whole patch is pointless :) and BTW, only the symlink case is about > file names. > > > > hw/9pfs/9p.c | 21 +++++++++++++++++++-- > > > 1 file changed, 19 insertions(+), 2 deletions(-) > > > > > > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c > > > index 7b1dfe4e47cb..dc65c3125006 100644 > > > --- a/hw/9pfs/9p.c > > > +++ b/hw/9pfs/9p.c > > > @@ -2150,6 +2150,11 @@ static void v9fs_create(void *opaque) > > > } > > > fidp->fid_type = P9_FID_DIR; > > > } else if (perm & P9_STAT_MODE_SYMLINK) { > > > + if (extension.data == NULL) { > > > + err = -EINVAL; > > > + goto out; > > > + } > > > > I realize that this part belongs to patch 1 actually: it is the implementation of > symbolic links that comes with 9P2000.u (different from the TSYMLINK request in > 9P2000.L). In which case, the hunk would have been: > > + if (name_is_illegal(extension.data)) { > + err = -EINVAL; > + goto out; > + } > > > POSIX specifically requires implementations to support creating a > > symlink whose target is the empty string. Linux doesn't [yet] permit > > it, but BSD does. On systems where creating such a symlink is legal, > > POSIX requires that such a symlink either be treated as "." if > > dereferenced, or be treated as ENOENT on attempt to dereference. But > > since such links can be created, readlink() should be able to read them > > without error. > > > > I would argue that we should NOT forbid empty symlinks on creation (but > > pass back any error from the underlying host OS); but instead check that > > dereferencing such a symlink behaves sanely if it was created. > > Meanwhile, a client should not be relying on the behavior (since Linux > > disobeys POSIX, portable clients should already be avoiding empty symlinks). > > > > http://austingroupbugs.net/view.php?id=649 > > > > Indeed, maybe we should let the backend decide if it allows symlink with > an empty target, since the target name is simply "stored" somewhere and > not used to create a new path. In which case, we should do the same with > v9fs_symlink(). And we would have two exceptions to the name_is_illegal() > helper, because we still want to avoid '/' in file names... > Thinking again... I guess '/' in names should result in ENOENT since it seems to be a common way to say something is wrong with a path... or we should have separate error paths between the '/'-in-names and the empty name cases. > On the other hand, we only support linux hosts where the call to symlink() > will fail with ENOENT and guests using the official linux kernel 9p client > never send an empty target... > > For the sake of simplicity, I'd rather have the target names to follow the > same rules as other file names, and return ENOENT directly (the link you > provide states it is a valid option). > > Peter, > > Since you suggested to do explicit error checking on empty file names, do > you have an opinion on the case of symlinks with an empty target ? > > > > @@ -2161,8 +2166,15 @@ static void v9fs_create(void *opaque) > > > } > > > v9fs_path_copy(&fidp->path, &path); > > > } else if (perm & P9_STAT_MODE_LINK) { > > > - int32_t ofid = atoi(extension.data); > > > - V9fsFidState *ofidp = get_fid(pdu, ofid); > > > + V9fsFidState *ofidp; > > > + > > > + if (extension.data == NULL) { > > > + err = -EINVAL; > > > + goto out; > > > + } > > > > Rejecting an empty destination on hard link or device creation, however, > > is indeed appropriate. > > > > In the case of hard links, extension.data is a FID, not a file name. > > In the case of device creation, extension.data is "type major minor", not > a file name again.
On 28 August 2016 at 13:21, Greg Kurz <groug@kaod.org> wrote: > Peter, > > Since you suggested to do explicit error checking on empty file names, do > you have an opinion on the case of symlinks with an empty target ? I have no strong opinion here. I was just aiming for the general principles of consistency and performing checks for invalid requests in the common layer code where possible. thanks -- PMM
On 08/28/2016 12:34 PM, Greg Kurz wrote: >> >> For the sake of simplicity, I'd rather have the target names to follow the >> same rules as other file names, and return ENOENT directly (the link you >> provide states it is a valid option). >> >> Peter, >> >> Since you suggested to do explicit error checking on empty file names, do >> you have an opinion on the case of symlinks with an empty target ? Failing with ENOENT for both "" and for "a/b" seems reasonable to me (a directory entry named "a/b" can never exist, just as "" cannot exist).
On Mon, 29 Aug 2016 14:35:07 -0500 Eric Blake <eblake@redhat.com> wrote: > On 08/28/2016 12:34 PM, Greg Kurz wrote: > > >> > >> For the sake of simplicity, I'd rather have the target names to follow the > >> same rules as other file names, and return ENOENT directly (the link you > >> provide states it is a valid option). > >> > >> Peter, > >> > >> Since you suggested to do explicit error checking on empty file names, do > >> you have an opinion on the case of symlinks with an empty target ? > > Failing with ENOENT for both "" and for "a/b" seems reasonable to me (a > directory entry named "a/b" can never exist, just as "" cannot exist). > But of course the target may still be a path with slashes :) And since we don't care for / until the link is dereferenced, I'm not so sure there is any checking to be done on the target name actually. -- Greg
diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c index 7b1dfe4e47cb..dc65c3125006 100644 --- a/hw/9pfs/9p.c +++ b/hw/9pfs/9p.c @@ -2150,6 +2150,11 @@ static void v9fs_create(void *opaque) } fidp->fid_type = P9_FID_DIR; } else if (perm & P9_STAT_MODE_SYMLINK) { + if (extension.data == NULL) { + err = -EINVAL; + goto out; + } + err = v9fs_co_symlink(pdu, fidp, &name, extension.data, -1 , &stbuf); if (err < 0) { @@ -2161,8 +2166,15 @@ static void v9fs_create(void *opaque) } v9fs_path_copy(&fidp->path, &path); } else if (perm & P9_STAT_MODE_LINK) { - int32_t ofid = atoi(extension.data); - V9fsFidState *ofidp = get_fid(pdu, ofid); + V9fsFidState *ofidp; + + if (extension.data == NULL) { + err = -EINVAL; + goto out; + } + + ofidp = get_fid(pdu, atoi(extension.data)); + if (ofidp == NULL) { err = -EINVAL; goto out; @@ -2188,6 +2200,11 @@ static void v9fs_create(void *opaque) uint32_t major, minor; mode_t nmode = 0; + if (extension.data == NULL) { + err = -EINVAL; + goto out; + } + if (sscanf(extension.data, "%c %u %u", &ctype, &major, &minor) != 3) { err = -errno; goto out;
A buggy guest using the 9p2000.u protocol can issue a create request and pass an empty string as the extension argument. This causes QEMU to crash in the case of a hard link or a special file, and leads to undefined behavior, depending on the backend, in the case of a symbolic link. This patch causes the request to fail with EINVAL in these scenarios. Signed-off-by: Greg Kurz <groug@kaod.org> --- hw/9pfs/9p.c | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-)