On Thu, 8 Feb 2018 19:00:17 +0100
<antonios.mota...@huawei.com> wrote:
> From: Antonios Motakis <antonios.mota...@huawei.com>
>
> 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/wiles/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 <antonios.mota...@huawei.com>
> ---
> hw/9pfs/9p.c | 67
> +++++++++++++++++++++++++++++++++++++++++++++++-------------
> hw/9pfs/9p.h | 1 +
> 2 files changed, 54 insertions(+), 14 deletions(-)
>
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index 85a1ed8..4da858f 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -573,10 +573,16 @@ 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) {
This check would then be performed by nearly all
requests. I'd rather set stbuf->st_dev at realize
time since we lstat() the root of the 9pfs share.
> + pdu->s->dev_id = stbuf->st_dev;
> + } else if (pdu->s->dev_id != stbuf->st_dev) {
> + return -ENOSYS;
Not sure ENOSYS is an appropriate return value for most guest
syscalls that would land here. Maybe ENOENT like if the path
doesn't exist instead ?
This being said, this st_dev related logic gets reverted in the
next patch IIUC... maybe this patch should just care to add a
return value to stat_to_qid() ?
> + }
> +
> memset(&qidp->path, 0, sizeof(qidp->path));
> size = MIN(sizeof(stbuf->st_ino), sizeof(qidp->path));
> memcpy(&qidp->path, &stbuf->st_ino, size);
> @@ -588,6 +594,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,
> @@ -600,7 +608,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;
> }
>
> @@ -831,7 +842,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;
> @@ -892,7 +906,7 @@ static int coroutine_fn stat_to_v9stat(V9fsPDU *pdu,
> V9fsPath *path,
> #define P9_STATS_ALL 0x00003fffULL /* 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));
> @@ -914,7 +928,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)
> @@ -1116,7 +1130,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) {
> @@ -1137,7 +1150,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) {
> @@ -1377,7 +1393,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));
> @@ -1477,7 +1496,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) {
> @@ -1587,7 +1609,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 out;
> @@ -2308,7 +2333,10 @@ static void coroutine_fn v9fs_create(void *opaque)
> }
> }
> 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 out;
> @@ -2365,7 +2393,10 @@ static void coroutine_fn v9fs_symlink(void *opaque)
> if (err < 0) {
> goto out;
> }
> - stat_to_qid(&stbuf, &qid);
> + err = stat_to_qid(pdu, &stbuf, &qid);
> + if (err < 0) {
> + goto out;
> + }
> err = pdu_marshal(pdu, offset, "Q", &qid);
> if (err < 0) {
> goto out;
> @@ -3033,7 +3064,10 @@ static void coroutine_fn v9fs_mknod(void *opaque)
> if (err < 0) {
> goto out;
> }
> - stat_to_qid(&stbuf, &qid);
> + err = stat_to_qid(pdu, &stbuf, &qid);
> + if (err < 0) {
> + goto out;
> + }
> err = pdu_marshal(pdu, offset, "Q", &qid);
> if (err < 0) {
> goto out;
> @@ -3191,7 +3225,10 @@ static void coroutine_fn v9fs_mkdir(void *opaque)
> if (err < 0) {
> goto out;
> }
> - stat_to_qid(&stbuf, &qid);
> + err = stat_to_qid(pdu, &stbuf, &qid);
> + if (err < 0) {
> + goto out;
> + }
> err = pdu_marshal(pdu, offset, "Q", &qid);
> if (err < 0) {
> goto out;
> @@ -3589,6 +3626,8 @@ int v9fs_device_realize_common(V9fsState *s, const
> V9fsTransport *t,
> goto out;
> }
>
> + s->dev_id = 0;
> +
> s->ctx.fst = &fse->fst;
> fsdev_throttle_init(s->ctx.fst);
>
> diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h
> index 5ced427..afb4ebd 100644
> --- a/hw/9pfs/9p.h
> +++ b/hw/9pfs/9p.h
> @@ -252,6 +252,7 @@ struct V9fsState
> Error *migration_blocker;
> V9fsConf fsconf;
> V9fsQID root_qid;
> + dev_t dev_id;
> };
>
> /* 9p2000.L open flags */
