Re: [Qemu-devel] [PATCH v4 2/5] 9p: Treat multiple devices on one export as an error
On Fri, 28 Jun 2019 14:36:41 +0200
Christian Schoenebeck wrote:
> On Donnerstag, 27. Juni 2019 19:26:22 CEST Greg Kurz wrote:
> > On Wed, 26 Jun 2019 20:30:41 +0200
> >
> > Christian Schoenebeck via Qemu-devel wrote:
> > > The QID path should uniquely identify a file. However, the
> > > inode of a file is currently used as the QID path, which
> > > on its own only uniquely identifies wiles within a device.
> >
> > s/wile/files
>
> Ah right. :)
>
> > > Here we track the device hosting the 9pfs share, in order
> > > to prevent security issues with QID path collisions from
> > > other devices.
> > >
> > > Signed-off-by: Antonios Motakis
> >
> > You should mention here the changes you made to the original patch.
>
> Got it. Will do for the other cases as well of course.
>
Cool.
> > > -static void stat_to_qid(const struct stat *stbuf, V9fsQID *qidp)
> > > +static int stat_to_qid(V9fsPDU *pdu, const struct stat *stbuf, V9fsQID
> > > *qidp)>
> > > {
> > >
> > > size_t size;
> > >
> > > +if (pdu->s->dev_id == 0) {
> > > +pdu->s->dev_id = stbuf->st_dev;
> >
> > st_dev should be captured in v9fs_device_realize_common() since we
> > lstat() the root there, instead of every request doing the check.
>
> Ok.
>
> > > +} else if (pdu->s->dev_id != stbuf->st_dev) {
> > > +error_report_once(
> > > +"9p: Multiple devices detected in same VirtFS export. "
> > > +"You must use a separate export for each device."
> > > +);
> > > +return -ENOSYS;
> >
> > This error is likely to end up as the return value of a
> > syscall in the guest and -ENOSYS usually means the syscall
> > isn't implemented, which is obviously not the case. Maybe
> > return -EPERM instead ?
>
> I would rather suggest -ENODEV. The entire device of the requested file/dir
> is
> not available on guest.
>
> -EPERM IMO rather motivates users looking for file system permission settings
> on individual files intead, and probably not checking the host's logs for the
> detailled error message.
>
-ENODEV is ok with me then.
> > > @@ -3633,6 +3674,8 @@ int v9fs_device_realize_common(V9fsState *s, const
> > > V9fsTransport *t,>
> > > goto out;
> > >
> > > }
> > >
> > > +s->dev_id = 0;
> > > +
> >
> > Set it to stat->st_dev after lstat() was called later in this function.
>
> I guesst you mean "earlier" not "later". The lstat() call is just before that
> dev_id initalization line. But in general your suggestion makes sense of
> course.
>
Oops... "earlier" indeed :)
> Best regards,
> Christian Schoenebeck
Re: [Qemu-devel] [PATCH v4 2/5] 9p: Treat multiple devices on one export as an error
On Donnerstag, 27. Juni 2019 19:26:22 CEST Greg Kurz wrote:
> On Wed, 26 Jun 2019 20:30:41 +0200
>
> Christian Schoenebeck via Qemu-devel wrote:
> > The QID path should uniquely identify a file. However, the
> > inode of a file is currently used as the QID path, which
> > on its own only uniquely identifies wiles within a device.
>
> s/wile/files
Ah right. :)
> > Here we track the device hosting the 9pfs share, in order
> > to prevent security issues with QID path collisions from
> > other devices.
> >
> > Signed-off-by: Antonios Motakis
>
> You should mention here the changes you made to the original patch.
Got it. Will do for the other cases as well of course.
> > -static void stat_to_qid(const struct stat *stbuf, V9fsQID *qidp)
> > +static int stat_to_qid(V9fsPDU *pdu, const struct stat *stbuf, V9fsQID
> > *qidp)>
> > {
> >
> > size_t size;
> >
> > +if (pdu->s->dev_id == 0) {
> > +pdu->s->dev_id = stbuf->st_dev;
>
> st_dev should be captured in v9fs_device_realize_common() since we
> lstat() the root there, instead of every request doing the check.
Ok.
> > +} else if (pdu->s->dev_id != stbuf->st_dev) {
> > +error_report_once(
> > +"9p: Multiple devices detected in same VirtFS export. "
> > +"You must use a separate export for each device."
> > +);
> > +return -ENOSYS;
>
> This error is likely to end up as the return value of a
> syscall in the guest and -ENOSYS usually means the syscall
> isn't implemented, which is obviously not the case. Maybe
> return -EPERM instead ?
I would rather suggest -ENODEV. The entire device of the requested file/dir is
not available on guest.
-EPERM IMO rather motivates users looking for file system permission settings
on individual files intead, and probably not checking the host's logs for the
detailled error message.
> > @@ -3633,6 +3674,8 @@ int v9fs_device_realize_common(V9fsState *s, const
> > V9fsTransport *t,>
> > goto out;
> >
> > }
> >
> > +s->dev_id = 0;
> > +
>
> Set it to stat->st_dev after lstat() was called later in this function.
I guesst you mean "earlier" not "later". The lstat() call is just before that
dev_id initalization line. But in general your suggestion makes sense of
course.
Best regards,
Christian Schoenebeck
Re: [Qemu-devel] [PATCH v4 2/5] 9p: Treat multiple devices on one export as an error
On Wed, 26 Jun 2019 20:30:41 +0200
Christian Schoenebeck via Qemu-devel wrote:
> The QID path should uniquely identify a file. However, the
> inode of a file is currently used as the QID path, which
> on its own only uniquely identifies wiles within a device.
s/wile/files
> Here we track the device hosting the 9pfs share, in order
> to prevent security issues with QID path collisions from
> other devices.
>
> Signed-off-by: Antonios Motakis
You should mention here the changes you made to the original patch.
> Signed-off-by: Christian Schoenebeck
> ---
> hw/9pfs/9p.c | 71
>
> hw/9pfs/9p.h | 1 +
> 2 files changed, 58 insertions(+), 14 deletions(-)
>
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index 586a6dccba..cbaa212625 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -572,10 +572,20 @@ static void coroutine_fn virtfs_reset(V9fsPDU *pdu)
> P9_STAT_MODE_SOCKET)
>
> /* This is the algorithm from ufs in spfs */
> -static void stat_to_qid(const struct stat *stbuf, V9fsQID *qidp)
> +static int stat_to_qid(V9fsPDU *pdu, const struct stat *stbuf, V9fsQID *qidp)
> {
> size_t size;
>
> +if (pdu->s->dev_id == 0) {
> +pdu->s->dev_id = stbuf->st_dev;
st_dev should be captured in v9fs_device_realize_common() since we
lstat() the root there, instead of every request doing the check.
> +} else if (pdu->s->dev_id != stbuf->st_dev) {
> +error_report_once(
> +"9p: Multiple devices detected in same VirtFS export. "
> +"You must use a separate export for each device."
> +);
> +return -ENOSYS;
This error is likely to end up as the return value of a
syscall in the guest and -ENOSYS usually means the syscall
isn't implemented, which is obviously not the case. Maybe
return -EPERM instead ?
> +}
> +
> memset(&qidp->path, 0, sizeof(qidp->path));
> size = MIN(sizeof(stbuf->st_ino), sizeof(qidp->path));
> memcpy(&qidp->path, &stbuf->st_ino, size);
> @@ -587,6 +597,8 @@ static void stat_to_qid(const struct stat *stbuf, V9fsQID
> *qidp)
> if (S_ISLNK(stbuf->st_mode)) {
> qidp->type |= P9_QID_TYPE_SYMLINK;
> }
> +
> +return 0;
> }
>
> static int coroutine_fn fid_to_qid(V9fsPDU *pdu, V9fsFidState *fidp,
> @@ -599,7 +611,10 @@ static int coroutine_fn fid_to_qid(V9fsPDU *pdu,
> V9fsFidState *fidp,
> if (err < 0) {
> return err;
> }
> -stat_to_qid(&stbuf, qidp);
> +err = stat_to_qid(pdu, &stbuf, qidp);
> +if (err < 0) {
> +return err;
> +}
> return 0;
> }
>
> @@ -830,7 +845,10 @@ static int coroutine_fn stat_to_v9stat(V9fsPDU *pdu,
> V9fsPath *path,
>
> memset(v9stat, 0, sizeof(*v9stat));
>
> -stat_to_qid(stbuf, &v9stat->qid);
> +err = stat_to_qid(pdu, stbuf, &v9stat->qid);
> +if (err < 0) {
> +return err;
> +}
> v9stat->mode = stat_to_v9mode(stbuf);
> v9stat->atime = stbuf->st_atime;
> v9stat->mtime = stbuf->st_mtime;
> @@ -891,7 +909,7 @@ static int coroutine_fn stat_to_v9stat(V9fsPDU *pdu,
> V9fsPath *path,
> #define P9_STATS_ALL 0x3fffULL /* Mask for All fields above */
>
>
> -static void stat_to_v9stat_dotl(V9fsState *s, const struct stat *stbuf,
> +static int stat_to_v9stat_dotl(V9fsPDU *pdu, const struct stat *stbuf,
> V9fsStatDotl *v9lstat)
> {
> memset(v9lstat, 0, sizeof(*v9lstat));
> @@ -913,7 +931,7 @@ static void stat_to_v9stat_dotl(V9fsState *s, const
> struct stat *stbuf,
> /* Currently we only support BASIC fields in stat */
> v9lstat->st_result_mask = P9_STATS_BASIC;
>
> -stat_to_qid(stbuf, &v9lstat->qid);
> +return stat_to_qid(pdu, stbuf, &v9lstat->qid);
> }
>
> static void print_sg(struct iovec *sg, int cnt)
> @@ -1115,7 +1133,6 @@ static void coroutine_fn v9fs_getattr(void *opaque)
> uint64_t request_mask;
> V9fsStatDotl v9stat_dotl;
> V9fsPDU *pdu = opaque;
> -V9fsState *s = pdu->s;
>
> retval = pdu_unmarshal(pdu, offset, "dq", &fid, &request_mask);
> if (retval < 0) {
> @@ -1136,7 +1153,10 @@ static void coroutine_fn v9fs_getattr(void *opaque)
> if (retval < 0) {
> goto out;
> }
> -stat_to_v9stat_dotl(s, &stbuf, &v9stat_dotl);
> +retval = stat_to_v9stat_dotl(pdu, &stbuf, &v9stat_dotl);
> +if (retval < 0) {
> +goto out;
> +}
>
> /* fill st_gen if requested and supported by underlying fs */
> if (request_mask & P9_STATS_GEN) {
> @@ -1381,7 +1401,10 @@ static void coroutine_fn v9fs_walk(void *opaque)
> if (err < 0) {
> goto out;
> }
> -stat_to_qid(&stbuf, &qid);
> +err = stat_to_qid(pdu, &stbuf, &qid);
> +if (err < 0) {
> +goto out;
> +}
> v9fs_path_copy(&dpath, &path);
> }
> memcpy(&q
[Qemu-devel] [PATCH v4 2/5] 9p: Treat multiple devices on one export as an error
The QID path should uniquely identify a file. However, the
inode of a file is currently used as the QID path, which
on its own only uniquely identifies wiles within a device.
Here we track the device hosting the 9pfs share, in order
to prevent security issues with QID path collisions from
other devices.
Signed-off-by: Antonios Motakis
Signed-off-by: Christian Schoenebeck
---
hw/9pfs/9p.c | 71
hw/9pfs/9p.h | 1 +
2 files changed, 58 insertions(+), 14 deletions(-)
diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 586a6dccba..cbaa212625 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -572,10 +572,20 @@ static void coroutine_fn virtfs_reset(V9fsPDU *pdu)
P9_STAT_MODE_SOCKET)
/* This is the algorithm from ufs in spfs */
-static void stat_to_qid(const struct stat *stbuf, V9fsQID *qidp)
+static int stat_to_qid(V9fsPDU *pdu, const struct stat *stbuf, V9fsQID *qidp)
{
size_t size;
+if (pdu->s->dev_id == 0) {
+pdu->s->dev_id = stbuf->st_dev;
+} else if (pdu->s->dev_id != stbuf->st_dev) {
+error_report_once(
+"9p: Multiple devices detected in same VirtFS export. "
+"You must use a separate export for each device."
+);
+return -ENOSYS;
+}
+
memset(&qidp->path, 0, sizeof(qidp->path));
size = MIN(sizeof(stbuf->st_ino), sizeof(qidp->path));
memcpy(&qidp->path, &stbuf->st_ino, size);
@@ -587,6 +597,8 @@ static void stat_to_qid(const struct stat *stbuf, V9fsQID
*qidp)
if (S_ISLNK(stbuf->st_mode)) {
qidp->type |= P9_QID_TYPE_SYMLINK;
}
+
+return 0;
}
static int coroutine_fn fid_to_qid(V9fsPDU *pdu, V9fsFidState *fidp,
@@ -599,7 +611,10 @@ static int coroutine_fn fid_to_qid(V9fsPDU *pdu,
V9fsFidState *fidp,
if (err < 0) {
return err;
}
-stat_to_qid(&stbuf, qidp);
+err = stat_to_qid(pdu, &stbuf, qidp);
+if (err < 0) {
+return err;
+}
return 0;
}
@@ -830,7 +845,10 @@ static int coroutine_fn stat_to_v9stat(V9fsPDU *pdu,
V9fsPath *path,
memset(v9stat, 0, sizeof(*v9stat));
-stat_to_qid(stbuf, &v9stat->qid);
+err = stat_to_qid(pdu, stbuf, &v9stat->qid);
+if (err < 0) {
+return err;
+}
v9stat->mode = stat_to_v9mode(stbuf);
v9stat->atime = stbuf->st_atime;
v9stat->mtime = stbuf->st_mtime;
@@ -891,7 +909,7 @@ static int coroutine_fn stat_to_v9stat(V9fsPDU *pdu,
V9fsPath *path,
#define P9_STATS_ALL 0x3fffULL /* Mask for All fields above */
-static void stat_to_v9stat_dotl(V9fsState *s, const struct stat *stbuf,
+static int stat_to_v9stat_dotl(V9fsPDU *pdu, const struct stat *stbuf,
V9fsStatDotl *v9lstat)
{
memset(v9lstat, 0, sizeof(*v9lstat));
@@ -913,7 +931,7 @@ static void stat_to_v9stat_dotl(V9fsState *s, const struct
stat *stbuf,
/* Currently we only support BASIC fields in stat */
v9lstat->st_result_mask = P9_STATS_BASIC;
-stat_to_qid(stbuf, &v9lstat->qid);
+return stat_to_qid(pdu, stbuf, &v9lstat->qid);
}
static void print_sg(struct iovec *sg, int cnt)
@@ -1115,7 +1133,6 @@ static void coroutine_fn v9fs_getattr(void *opaque)
uint64_t request_mask;
V9fsStatDotl v9stat_dotl;
V9fsPDU *pdu = opaque;
-V9fsState *s = pdu->s;
retval = pdu_unmarshal(pdu, offset, "dq", &fid, &request_mask);
if (retval < 0) {
@@ -1136,7 +1153,10 @@ static void coroutine_fn v9fs_getattr(void *opaque)
if (retval < 0) {
goto out;
}
-stat_to_v9stat_dotl(s, &stbuf, &v9stat_dotl);
+retval = stat_to_v9stat_dotl(pdu, &stbuf, &v9stat_dotl);
+if (retval < 0) {
+goto out;
+}
/* fill st_gen if requested and supported by underlying fs */
if (request_mask & P9_STATS_GEN) {
@@ -1381,7 +1401,10 @@ static void coroutine_fn v9fs_walk(void *opaque)
if (err < 0) {
goto out;
}
-stat_to_qid(&stbuf, &qid);
+err = stat_to_qid(pdu, &stbuf, &qid);
+if (err < 0) {
+goto out;
+}
v9fs_path_copy(&dpath, &path);
}
memcpy(&qids[name_idx], &qid, sizeof(qid));
@@ -1483,7 +1506,10 @@ static void coroutine_fn v9fs_open(void *opaque)
if (err < 0) {
goto out;
}
-stat_to_qid(&stbuf, &qid);
+err = stat_to_qid(pdu, &stbuf, &qid);
+if (err < 0) {
+goto out;
+}
if (S_ISDIR(stbuf.st_mode)) {
err = v9fs_co_opendir(pdu, fidp);
if (err < 0) {
@@ -1593,7 +1619,10 @@ static void coroutine_fn v9fs_lcreate(void *opaque)
fidp->flags |= FID_NON_RECLAIMABLE;
}
iounit = get_iounit(pdu, &fidp->path);
-stat_to_qid(&stbuf, &qid);
+err = stat_to_qid(pdu, &stbuf, &qid);
+if (err < 0) {
+goto out;
+}
err = pdu_marshal(pdu, offset, "Qd", &qid, iounit);
if (err < 0) {
goto
