| Message ID | 147222402095.18925.396915168302239352.stgit@bahia.lan (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 08/26/2016 10:07 AM, Greg Kurz wrote: > Empty path components don't make sense and may cause undefined behavior, > depending on the backend. > > Also, the walk request described in the 9P spec [1] clearly shows that > the client is supposed to send individual path components: the official > linux client never sends portions of path containing the / character for > example. > > Moreover, the 9P spec [2] also states that a system can decide to restrict > the set of supported characters used in path components, with an explicit > mention "to remove slashes from name components". > > This patch introduces a new name_is_illegal() helper that checks the > names sent by the client are not empty and don't contain unwanted chars. > Since 9pfs is only supported on linux hosts, only the / character is > checked at the moment. When support for other hosts (AKA. win32) is added, > other chars may need to be blacklisted as well. > > If a client sends an illegal path component, the request will fail and > EINVAL is returned to the client. > > [1] http://man.cat-v.org/plan_9/5/walk > [2] http://man.cat-v.org/plan_9/5/intro > > Suggested-by: Peter Maydell <peter.maydell@linaro.org> > Signed-off-by: Greg Kurz <groug@kaod.org> > --- > hw/9pfs/9p.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 56 insertions(+) > > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c > index b6b02b46a9da..dba11773699b 100644 > --- a/hw/9pfs/9p.c > +++ b/hw/9pfs/9p.c > @@ -1256,6 +1256,11 @@ static int v9fs_walk_marshal(V9fsPDU *pdu, uint16_t nwnames, V9fsQID *qids) > return offset; > } > > +static bool name_is_illegal(const char *name) > +{ > + return name == NULL || strchr(name, '/') != NULL; Is anyone actually passing NULL? And the commit message says you are blacklisting the empty string "", but that is not what you did here. Depending on whether callers can even pass NULL, you may be able to just s/name == NULL/!*name/
On Fri, 26 Aug 2016 13:33:28 -0500 Eric Blake <eblake@redhat.com> wrote: > On 08/26/2016 10:07 AM, Greg Kurz wrote: > > Empty path components don't make sense and may cause undefined behavior, > > depending on the backend. > > > > Also, the walk request described in the 9P spec [1] clearly shows that > > the client is supposed to send individual path components: the official > > linux client never sends portions of path containing the / character for > > example. > > > > Moreover, the 9P spec [2] also states that a system can decide to restrict > > the set of supported characters used in path components, with an explicit > > mention "to remove slashes from name components". > > > > This patch introduces a new name_is_illegal() helper that checks the > > names sent by the client are not empty and don't contain unwanted chars. > > Since 9pfs is only supported on linux hosts, only the / character is > > checked at the moment. When support for other hosts (AKA. win32) is added, > > other chars may need to be blacklisted as well. > > > > If a client sends an illegal path component, the request will fail and > > EINVAL is returned to the client. > > > > [1] http://man.cat-v.org/plan_9/5/walk > > [2] http://man.cat-v.org/plan_9/5/intro > > > > Suggested-by: Peter Maydell <peter.maydell@linaro.org> > > Signed-off-by: Greg Kurz <groug@kaod.org> > > --- > > hw/9pfs/9p.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 56 insertions(+) > > > > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c > > index b6b02b46a9da..dba11773699b 100644 > > --- a/hw/9pfs/9p.c > > +++ b/hw/9pfs/9p.c > > @@ -1256,6 +1256,11 @@ static int v9fs_walk_marshal(V9fsPDU *pdu, uint16_t nwnames, V9fsQID *qids) > > return offset; > > } > > > > +static bool name_is_illegal(const char *name) > > +{ > > + return name == NULL || strchr(name, '/') != NULL; > > Is anyone actually passing NULL? And the commit message says you are > blacklisting the empty string "", but that is not what you did here. > Depending on whether callers can even pass NULL, you may be able to just > s/name == NULL/!*name/ > Oops you're right, v9fs_iov_vunmarshal() always allocates and sets str->data, even for empty strings... so I guess this cannot be call with NULL and your suggestion is the way to go.
diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c index b6b02b46a9da..dba11773699b 100644 --- a/hw/9pfs/9p.c +++ b/hw/9pfs/9p.c @@ -1256,6 +1256,11 @@ static int v9fs_walk_marshal(V9fsPDU *pdu, uint16_t nwnames, V9fsQID *qids) return offset; } +static bool name_is_illegal(const char *name) +{ + return name == NULL || strchr(name, '/') != NULL; +} + static void v9fs_walk(void *opaque) { int name_idx; @@ -1289,6 +1294,10 @@ static void v9fs_walk(void *opaque) if (err < 0) { goto out_nofid; } + if (name_is_illegal(wnames[i].data)) { + err = -EINVAL; + goto out_nofid; + } offset += err; } } else if (nwnames > P9_MAXWELEM) { @@ -1483,6 +1492,11 @@ static void v9fs_lcreate(void *opaque) } trace_v9fs_lcreate(pdu->tag, pdu->id, dfid, flags, mode, gid); + if (name_is_illegal(name.data)) { + err = -EINVAL; + goto out_nofid; + } + fidp = get_fid(pdu, dfid); if (fidp == NULL) { err = -ENOENT; @@ -2077,6 +2091,11 @@ static void v9fs_create(void *opaque) } trace_v9fs_create(pdu->tag, pdu->id, fid, name.data, perm, mode); + if (name_is_illegal(name.data)) { + err = -EINVAL; + goto out_nofid; + } + fidp = get_fid(pdu, fid); if (fidp == NULL) { err = -EINVAL; @@ -2242,6 +2261,11 @@ static void v9fs_symlink(void *opaque) } trace_v9fs_symlink(pdu->tag, pdu->id, dfid, name.data, symname.data, gid); + if (name_is_illegal(name.data) || name_is_illegal(symname.data)) { + err = -EINVAL; + goto out_nofid; + } + dfidp = get_fid(pdu, dfid); if (dfidp == NULL) { err = -EINVAL; @@ -2316,6 +2340,11 @@ static void v9fs_link(void *opaque) } trace_v9fs_link(pdu->tag, pdu->id, dfid, oldfid, name.data); + if (name_is_illegal(name.data)) { + err = -EINVAL; + goto out_nofid; + } + dfidp = get_fid(pdu, dfid); if (dfidp == NULL) { err = -ENOENT; @@ -2398,6 +2427,12 @@ static void v9fs_unlinkat(void *opaque) if (err < 0) { goto out_nofid; } + + if (name_is_illegal(name.data)) { + err = -EINVAL; + goto out_nofid; + } + dfidp = get_fid(pdu, dfid); if (dfidp == NULL) { err = -EINVAL; @@ -2504,6 +2539,12 @@ static void v9fs_rename(void *opaque) if (err < 0) { goto out_nofid; } + + if (name_is_illegal(name.data)) { + err = -EINVAL; + goto out_nofid; + } + fidp = get_fid(pdu, fid); if (fidp == NULL) { err = -ENOENT; @@ -2616,6 +2657,11 @@ static void v9fs_renameat(void *opaque) goto out_err; } + if (name_is_illegal(old_name.data) || name_is_illegal(new_name.data)) { + err = -EINVAL; + goto out_err; + } + v9fs_path_write_lock(s); err = v9fs_complete_renameat(pdu, olddirfid, &old_name, newdirfid, &new_name); @@ -2826,6 +2872,11 @@ static void v9fs_mknod(void *opaque) } trace_v9fs_mknod(pdu->tag, pdu->id, fid, mode, major, minor); + if (name_is_illegal(name.data)) { + err = -EINVAL; + goto out_nofid; + } + fidp = get_fid(pdu, fid); if (fidp == NULL) { err = -ENOENT; @@ -2977,6 +3028,11 @@ static void v9fs_mkdir(void *opaque) } trace_v9fs_mkdir(pdu->tag, pdu->id, fid, name.data, mode, gid); + if (name_is_illegal(name.data)) { + err = -EINVAL; + goto out_nofid; + } + fidp = get_fid(pdu, fid); if (fidp == NULL) { err = -ENOENT;
Empty path components don't make sense and may cause undefined behavior, depending on the backend. Also, the walk request described in the 9P spec [1] clearly shows that the client is supposed to send individual path components: the official linux client never sends portions of path containing the / character for example. Moreover, the 9P spec [2] also states that a system can decide to restrict the set of supported characters used in path components, with an explicit mention "to remove slashes from name components". This patch introduces a new name_is_illegal() helper that checks the names sent by the client are not empty and don't contain unwanted chars. Since 9pfs is only supported on linux hosts, only the / character is checked at the moment. When support for other hosts (AKA. win32) is added, other chars may need to be blacklisted as well. If a client sends an illegal path component, the request will fail and EINVAL is returned to the client. [1] http://man.cat-v.org/plan_9/5/walk [2] http://man.cat-v.org/plan_9/5/intro Suggested-by: Peter Maydell <peter.maydell@linaro.org> Signed-off-by: Greg Kurz <groug@kaod.org> --- hw/9pfs/9p.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 56 insertions(+)