On Fri, 28 Jun 2019 14:36:41 +0200
Christian Schoenebeck <qemu_...@crudebyte.com> 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 <qemu-devel@nongnu.org> 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 <antonios.mota...@huawei.com>
> >
> > 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
