Message ID | 154226612321.706352.2726376089201743065.stgit@bahia.lan |
---|---|
State | New |
Headers | show |
Series | 9p: take write lock on fid path updates | expand |
+-- On Thu, 15 Nov 2018, Greg Kurz wrote --+
| Recent commit 5b76ef50f62079a fixed a race where v9fs_co_open2() could
| possibly overwrite a fid path with v9fs_path_copy() while it is being
| accessed by some other thread, ie, use-after-free that can be detected
| by ASAN with a custom 9p client.
|
| It turns out that the same can happen at several locations where
| v9fs_path_copy() is used to set the fid path. The fix is again to
| take the write lock.
|
| Cc: P J P <ppandit@redhat.com>
| Reported-by: zhibin hu <noirfate@gmail.com>
| Signed-off-by: Greg Kurz <groug@kaod.org>
| ---
| hw/9pfs/9p.c | 15 +++++++++++++++
| 1 file changed, 15 insertions(+)
|
| diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
| index eef289e394d4..267a25533b77 100644
| --- a/hw/9pfs/9p.c
| +++ b/hw/9pfs/9p.c
| @@ -1391,7 +1391,9 @@ static void coroutine_fn v9fs_walk(void *opaque)
| err = -EINVAL;
| goto out;
| }
| + v9fs_path_write_lock(s);
| v9fs_path_copy(&fidp->path, &path);
| + v9fs_path_unlock(s);
| } else {
| newfidp = alloc_fid(s, newfid);
| if (newfidp == NULL) {
| @@ -2160,6 +2162,7 @@ static void coroutine_fn v9fs_create(void *opaque)
| V9fsString extension;
| int iounit;
| V9fsPDU *pdu = opaque;
| + V9fsState *s = pdu->s;
|
| v9fs_path_init(&path);
| v9fs_string_init(&name);
| @@ -2200,7 +2203,9 @@ static void coroutine_fn v9fs_create(void *opaque)
| if (err < 0) {
| goto out;
| }
| + v9fs_path_write_lock(s);
| v9fs_path_copy(&fidp->path, &path);
| + v9fs_path_unlock(s);
| err = v9fs_co_opendir(pdu, fidp);
| if (err < 0) {
| goto out;
| @@ -2216,7 +2221,9 @@ static void coroutine_fn v9fs_create(void *opaque)
| if (err < 0) {
| goto out;
| }
| + v9fs_path_write_lock(s);
| v9fs_path_copy(&fidp->path, &path);
| + v9fs_path_unlock(s);
| } else if (perm & P9_STAT_MODE_LINK) {
| int32_t ofid = atoi(extension.data);
| V9fsFidState *ofidp = get_fid(pdu, ofid);
| @@ -2234,7 +2241,9 @@ static void coroutine_fn v9fs_create(void *opaque)
| fidp->fid_type = P9_FID_NONE;
| goto out;
| }
| + v9fs_path_write_lock(s);
| v9fs_path_copy(&fidp->path, &path);
| + v9fs_path_unlock(s);
| err = v9fs_co_lstat(pdu, &fidp->path, &stbuf);
| if (err < 0) {
| fidp->fid_type = P9_FID_NONE;
| @@ -2272,7 +2281,9 @@ static void coroutine_fn v9fs_create(void *opaque)
| if (err < 0) {
| goto out;
| }
| + v9fs_path_write_lock(s);
| v9fs_path_copy(&fidp->path, &path);
| + v9fs_path_unlock(s);
| } else if (perm & P9_STAT_MODE_NAMED_PIPE) {
| err = v9fs_co_mknod(pdu, fidp, &name, fidp->uid, -1,
| 0, S_IFIFO | (perm & 0777), &stbuf);
| @@ -2283,7 +2294,9 @@ static void coroutine_fn v9fs_create(void *opaque)
| if (err < 0) {
| goto out;
| }
| + v9fs_path_write_lock(s);
| v9fs_path_copy(&fidp->path, &path);
| + v9fs_path_unlock(s);
| } else if (perm & P9_STAT_MODE_SOCKET) {
| err = v9fs_co_mknod(pdu, fidp, &name, fidp->uid, -1,
| 0, S_IFSOCK | (perm & 0777), &stbuf);
| @@ -2294,7 +2307,9 @@ static void coroutine_fn v9fs_create(void *opaque)
| if (err < 0) {
| goto out;
| }
| + v9fs_path_write_lock(s);
| v9fs_path_copy(&fidp->path, &path);
| + v9fs_path_unlock(s);
| } else {
| err = v9fs_co_open2(pdu, fidp, &name, -1,
| omode_to_uflags(mode)|O_CREAT, perm, &stbuf);
|
Looks okay.
Reviewed-by: Prasad J Pandit <pjp@fedoraproject.org>
Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c index eef289e394d4..267a25533b77 100644 --- a/hw/9pfs/9p.c +++ b/hw/9pfs/9p.c @@ -1391,7 +1391,9 @@ static void coroutine_fn v9fs_walk(void *opaque) err = -EINVAL; goto out; } + v9fs_path_write_lock(s); v9fs_path_copy(&fidp->path, &path); + v9fs_path_unlock(s); } else { newfidp = alloc_fid(s, newfid); if (newfidp == NULL) { @@ -2160,6 +2162,7 @@ static void coroutine_fn v9fs_create(void *opaque) V9fsString extension; int iounit; V9fsPDU *pdu = opaque; + V9fsState *s = pdu->s; v9fs_path_init(&path); v9fs_string_init(&name); @@ -2200,7 +2203,9 @@ static void coroutine_fn v9fs_create(void *opaque) if (err < 0) { goto out; } + v9fs_path_write_lock(s); v9fs_path_copy(&fidp->path, &path); + v9fs_path_unlock(s); err = v9fs_co_opendir(pdu, fidp); if (err < 0) { goto out; @@ -2216,7 +2221,9 @@ static void coroutine_fn v9fs_create(void *opaque) if (err < 0) { goto out; } + v9fs_path_write_lock(s); v9fs_path_copy(&fidp->path, &path); + v9fs_path_unlock(s); } else if (perm & P9_STAT_MODE_LINK) { int32_t ofid = atoi(extension.data); V9fsFidState *ofidp = get_fid(pdu, ofid); @@ -2234,7 +2241,9 @@ static void coroutine_fn v9fs_create(void *opaque) fidp->fid_type = P9_FID_NONE; goto out; } + v9fs_path_write_lock(s); v9fs_path_copy(&fidp->path, &path); + v9fs_path_unlock(s); err = v9fs_co_lstat(pdu, &fidp->path, &stbuf); if (err < 0) { fidp->fid_type = P9_FID_NONE; @@ -2272,7 +2281,9 @@ static void coroutine_fn v9fs_create(void *opaque) if (err < 0) { goto out; } + v9fs_path_write_lock(s); v9fs_path_copy(&fidp->path, &path); + v9fs_path_unlock(s); } else if (perm & P9_STAT_MODE_NAMED_PIPE) { err = v9fs_co_mknod(pdu, fidp, &name, fidp->uid, -1, 0, S_IFIFO | (perm & 0777), &stbuf); @@ -2283,7 +2294,9 @@ static void coroutine_fn v9fs_create(void *opaque) if (err < 0) { goto out; } + v9fs_path_write_lock(s); v9fs_path_copy(&fidp->path, &path); + v9fs_path_unlock(s); } else if (perm & P9_STAT_MODE_SOCKET) { err = v9fs_co_mknod(pdu, fidp, &name, fidp->uid, -1, 0, S_IFSOCK | (perm & 0777), &stbuf); @@ -2294,7 +2307,9 @@ static void coroutine_fn v9fs_create(void *opaque) if (err < 0) { goto out; } + v9fs_path_write_lock(s); v9fs_path_copy(&fidp->path, &path); + v9fs_path_unlock(s); } else { err = v9fs_co_open2(pdu, fidp, &name, -1, omode_to_uflags(mode)|O_CREAT, perm, &stbuf);
Recent commit 5b76ef50f62079a fixed a race where v9fs_co_open2() could possibly overwrite a fid path with v9fs_path_copy() while it is being accessed by some other thread, ie, use-after-free that can be detected by ASAN with a custom 9p client. It turns out that the same can happen at several locations where v9fs_path_copy() is used to set the fid path. The fix is again to take the write lock. Cc: P J P <ppandit@redhat.com> Reported-by: zhibin hu <noirfate@gmail.com> Signed-off-by: Greg Kurz <groug@kaod.org> --- hw/9pfs/9p.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)