Skip to content

Q
qemu

openeuler / qemu

通知 10
Star 0
Fork 0
Q
qemu

体验新版 GitCode，发现更多精彩内容 >>

切换分支/标签
  1. 25 5月, 2017 8 次提交
    • G
      9pfs: local: metadata file for the VirtFS root · 81ffbf5a
      由 Greg Kurz 提交于 
      When using the mapped-file security, credentials are stored in a metadata
directory located in the parent directory. This is okay for all paths with
the notable exception of the root path, since we don't want and probably
can't create a metadata directory above the virtfs directory on the host.

This patch introduces a dedicated metadata file, sitting in the virtfs root
for this purpose. It relies on the fact that the "." name necessarily refers
to the virtfs root.

As for the metadata directory, we don't want the client to see this file.
The current code only cares for readdir() but there are many other places
to fix actually. The filtering logic is hence put in a separate function.

Before:

# ls -ld
drwxr-xr-x. 3 greg greg 4096 May  5 12:49 .
# chown root.root .
chown: changing ownership of '.': Is a directory
# ls -ld
drwxr-xr-x. 3 greg greg 4096 May  5 12:49 .

After:

# ls -ld
drwxr-xr-x. 3 greg greg 4096 May  5 12:49 .
# chown root.root .
# ls -ld
drwxr-xr-x. 3 root root 4096 May  5 12:50 .

and from the host:

ls -al .virtfs_metadata_root
-rwx------. 1 greg greg 26 May  5 12:50 .virtfs_metadata_root
$ cat .virtfs_metadata_root
virtfs.uid=0
virtfs.gid=0
Reported-by: NLeo Gaspard <leo@gaspard.io>
Signed-off-by: NGreg Kurz <groug@kaod.org>
Reviewed-by: NEric Blake <eblake@redhat.com>
Tested-by: NLeo Gaspard <leo@gaspard.io>
[groug: work around a patchew false positive in
        local_set_mapped_file_attrat()]
      81ffbf5a
    • G
      9pfs: local: simplify file opening · 3dbcf273
      由 Greg Kurz 提交于 
      The logic to open a path currently sits between local_open_nofollow() and
the relative_openat_nofollow() helper, which has no other user.

For the sake of clarity, this patch moves all the code of the helper into
its unique caller. While here we also:
- drop the code to skip leading "/" because the backend isn't supposed to
  pass anything but relative paths without consecutive slashes. The assert()
  is kept because we really don't want a buggy backend to pass an absolute
  path to openat().
- use strchrnul() to get a simpler code. This is ok since virtfs is for
  linux+glibc hosts only.
- don't dup() the initial directory and add an assert() to ensure we don't
  return the global mountfd to the caller. BTW, this would mean that the
  caller passed an empty path, which isn't supposed to happen either.
Signed-off-by: NGreg Kurz <groug@kaod.org>
Reviewed-by: NEric Blake <eblake@redhat.com>
[groug: fixed typos in changelog]
      3dbcf273
    • G
      9pfs: local: resolve special directories in paths · f57f5878
      由 Greg Kurz 提交于 
      When using the mapped-file security mode, the creds of a path /foo/bar
are stored in the /foo/.virtfs_metadata/bar file. This is okay for all
paths unless they end with '.' or '..', because we cannot create the
corresponding file in the metadata directory.

This patch ensures that '.' and '..' are resolved in all paths.

The core code only passes path elements (no '/') to the backend, with
the notable exception of the '/' path, which refers to the virtfs root.
This patch preserves the current behavior of converting it to '.' so
that it can be passed to "*at()" syscalls ('/' would mean the host root).
Signed-off-by: NGreg Kurz <groug@kaod.org>
Reviewed-by: NEric Blake <eblake@redhat.com>
      f57f5878
    • G
      9pfs: check return value of v9fs_co_name_to_path() · 4fa62005
      由 Greg Kurz 提交于 
      These v9fs_co_name_to_path() call sites have always been around. I guess
no care was taken to check the return value because the name_to_path
operation could never fail at the time. This is no longer true: the
handle and synth backends can already fail this operation, and so will the
local backend soon.
Signed-off-by: NGreg Kurz <groug@kaod.org>
Reviewed-by: NEric Blake <eblake@redhat.com>
      4fa62005
    • G
      9pfs: assume utimensat() and futimens() are present · 24df3371
      由 Greg Kurz 提交于 
      The utimensat() and futimens() syscalls have been around for ages (ie,
glibc 2.6 and linux 2.6.22), and the decision was already taken to
switch to utimensat() anyway when fixing CVE-2016-9602 in 2.9.
Signed-off-by: NGreg Kurz <groug@kaod.org>
Reviewed-by: NEric Blake <eblake@redhat.com>
      24df3371
    • G
      9pfs: local: fix unlink of alien files in mapped-file mode · 6a87e792
      由 Greg Kurz 提交于 
      When trying to remove a file from a directory, both created in non-mapped
mode, the file remains and EBADF is returned to the guest.

This is a regression introduced by commit "df4938a6 9pfs: local:
unlinkat: don't follow symlinks" when fixing CVE-2016-9602. It changed the
way we unlink the metadata file from

    ret = remove("$dir/.virtfs_metadata/$name");
    if (ret < 0 && errno != ENOENT) {
         /* Error out */
    }
    /* Ignore absence of metadata */

to

    fd = openat("$dir/.virtfs_metadata")
    unlinkat(fd, "$name")
    if (ret < 0 && errno != ENOENT) {
         /* Error out */
    }
    /* Ignore absence of metadata */

If $dir was created in non-mapped mode, openat() fails with ENOENT and
we pass -1 to unlinkat(), which fails in turn with EBADF.

We just need to check the return of openat() and ignore ENOENT, in order
to restore the behaviour we had with remove().
Signed-off-by: NGreg Kurz <groug@kaod.org>
Reviewed-by: NEric Blake <eblake@redhat.com>
[groug: rewrote the comments as suggested by Eric]
      6a87e792
    • G
      9pfs: drop pdu_push_and_notify() · a17d8659
      由 Greg Kurz 提交于 
      Only pdu_complete() needs to notify the client that a request has completed.
Signed-off-by: NGreg Kurz <groug@kaod.org>
Reviewed-by: NStefano Stabellini <sstabellini@kernel.org>
      a17d8659
    • G
      virtio-9p/xen-9p: move 9p specific bits to core 9p code · 506f3275
      由 Greg Kurz 提交于 
      These bits aren't related to the transport so let's move them to the core
code.
Signed-off-by: NGreg Kurz <groug@kaod.org>
Reviewed-by: NStefano Stabellini <sstabellini@kernel.org>
      506f3275
  3. 17 5月, 2017 3 次提交
  5. 15 5月, 2017 1 次提交
    • G
      9pfs: local: forbid client access to metadata (CVE-2017-7493) · 7a95434e
      由 Greg Kurz 提交于 
      When using the mapped-file security mode, we shouldn't let the client mess
with the metadata. The current code already tries to hide the metadata dir
from the client by skipping it in local_readdir(). But the client can still
access or modify it through several other operations. This can be used to
escalate privileges in the guest.

Affected backend operations are:
- local_mknod()
- local_mkdir()
- local_open2()
- local_symlink()
- local_link()
- local_unlinkat()
- local_renameat()
- local_rename()
- local_name_to_path()

Other operations are safe because they are only passed a fid path, which
is computed internally in local_name_to_path().

This patch converts all the functions listed above to fail and return
EINVAL when being passed the name of the metadata dir. This may look
like a poor choice for errno, but there's no such thing as an illegal
path name on Linux and I could not think of anything better.

This fixes CVE-2017-7493.
Reported-by: NLeo Gaspard <leo@gaspard.io>
Signed-off-by: NGreg Kurz <groug@kaod.org>
Reviewed-by: NEric Blake <eblake@redhat.com>
      7a95434e
  7. 26 4月, 2017 6 次提交
  9. 22 4月, 2017 1 次提交
  11. 18 4月, 2017 1 次提交
    • G
      9pfs: local: set the path of the export root to "." · 9c6b899f
      由 Greg Kurz 提交于 
      The local backend was recently converted to using "at*()" syscalls in order
to ensure all accesses happen below the shared directory. This requires that
we only pass relative paths, otherwise the dirfd argument to the "at*()"
syscalls is ignored and the path is treated as an absolute path in the host.
This is actually the case for paths in all fids, with the notable exception
of the root fid, whose path is "/". This causes the following backend ops to
act on the "/" directory of the host instead of the virtfs shared directory
when the export root is involved:
- lstat
- chmod
- chown
- utimensat

ie, chmod /9p_mount_point in the guest will be converted to chmod / in the
host for example. This could cause security issues with a privileged QEMU.

All "*at()" syscalls are being passed an open file descriptor. In the case
of the export root, this file descriptor points to the path in the host that
was passed to -fsdev.

The fix is thus as simple as changing the path of the export root fid to be
"." instead of "/".

This is CVE-2017-7471.

Cc: qemu-stable@nongnu.org
Reported-by: NLéo Gaspard <leo@gaspard.io>
Signed-off-by: NGreg Kurz <groug@kaod.org>
Reviewed-by: NEric Blake <eblake@redhat.com>
Signed-off-by: NPeter Maydell <peter.maydell@linaro.org>
      9c6b899f
  13. 10 4月, 2017 1 次提交
  15. 05 4月, 2017 2 次提交
    • G
      9pfs: clear migration blocker at session reset · 6d54af0e
      由 Greg Kurz 提交于 
      The migration blocker survives a device reset: if the guest mounts a 9p
share and then gets rebooted with system_reset, it will be unmigratable
until it remounts and umounts the 9p share again.

This happens because the migration blocker is supposed to be cleared when
we put the last reference on the root fid, but virtfs_reset() wrongly calls
free_fid() instead of put_fid().

This patch fixes virtfs_reset() so that it honor the way fids are supposed
to be manipulated: first get a reference and later put it back when you're
done.
Signed-off-by: NGreg Kurz <groug@kaod.org>
Reviewed-by: NLi Qiang <liqiang6-s@360.cn>
      6d54af0e
    • G
      9pfs: fix multiple flush for same request · 18adde86
      由 Greg Kurz 提交于 
      If a client tries to flush the same outstanding request several times, only
the first flush completes. Subsequent ones keep waiting for the request
completion in v9fs_flush() and, therefore, leak a PDU. This will cause QEMU
to hang when draining active PDUs the next time the device is reset.

Let have each flush request wake up the next one if any. The last waiter
frees the cancelled PDU.
Signed-off-by: NGreg Kurz <groug@kaod.org>
Reviewed-by: NEric Blake <eblake@redhat.com>
      18adde86
  17. 28 3月, 2017 1 次提交
    • L
      9pfs: fix file descriptor leak · d63fb193
      由 Li Qiang 提交于 
      The v9fs_create() and v9fs_lcreate() functions are used to create a file
on the backend and to associate it to a fid. The fid shouldn't be already
in-use, otherwise both functions may silently leak a file descriptor or
allocated memory. The current code doesn't check that.

This patch ensures that the fid isn't already associated to anything
before using it.
Signed-off-by: NLi Qiang <liqiang6-s@360.cn>
(reworded the changelog, Greg Kurz)
Signed-off-by: NGreg Kurz <groug@kaod.org>
      d63fb193
  19. 21 3月, 2017 2 次提交
    • G
      9pfs: proxy: assert if unmarshal fails · 262169ab
      由 Greg Kurz 提交于 
      Replies from the virtfs proxy are made up of a fixed-size header (8 bytes)
and a payload of variable size (maximum 64kb). When receiving a reply,
the proxy backend first reads the whole header and then unmarshals it.
If the header is okay, it then does the same operation with the payload.

Since the proxy backend uses a pre-allocated buffer which has enough room
for a header and the maximum payload size, marshalling should never fail
with fixed size arguments. Any error here is likely to result from a more
serious corruption in QEMU and we'd better dump core right away.

This patch adds error checks where they are missing and converts the
associated error paths into assertions.

This should also address Coverity's complaints CID 1348519 and CID 1348520,
about not always checking the return value of proxy_unmarshal().
Signed-off-by: NGreg Kurz <groug@kaod.org>
Reviewed-by: NPhilippe Mathieu-Daudé <f4bug@amsat.org>
      262169ab
    • G
      9pfs: don't try to flush self and avoid QEMU hang on reset · d5f2af7b
      由 Greg Kurz 提交于 
      According to the 9P spec [*], when a client wants to cancel a pending I/O
request identified by a given tag (uint16), it must send a Tflush message
and wait for the server to respond with a Rflush message before reusing this
tag for another I/O. The server may still send a completion message for the
I/O if it wasn't actually cancelled but the Rflush message must arrive after
that.

QEMU hence waits for the flushed PDU to complete before sending the Rflush
message back to the client.

If a client sends 'Tflush tag oldtag' and tag == oldtag, QEMU will then
allocate a PDU identified by tag, find it in the PDU list and wait for
this same PDU to complete... i.e. wait for a completion that will never
happen. This causes a tag and ring slot leak in the guest, and a PDU
leak in QEMU, all of them limited by the maximal number of PDUs (128).
But, worse, this causes QEMU to hang on device reset since v9fs_reset()
wants to drain all pending I/O.

This insane behavior is likely to denote a bug in the client, and it would
deserve an Rerror message to be sent back. Unfortunately, the protocol
allows it and requires all flush requests to suceed (only a Tflush response
is expected).

The only option is to detect when we have to handle a self-referencing
flush request and report success to the client right away.

[*] http://man.cat-v.org/plan_9/5/flushReported-by: NAl Viro <viro@ZenIV.linux.org.uk>
Signed-off-by: NGreg Kurz <groug@kaod.org>
      d5f2af7b
  21. 07 3月, 2017 6 次提交
  23. 28 2月, 2017 8 次提交
    • G
      9pfs: local: drop unused code · c23d5f1d
      由 Greg Kurz 提交于 
      Now that the all callbacks have been converted to use "at" syscalls, we
can drop this code.
Signed-off-by: NGreg Kurz <groug@kaod.org>
Reviewed-by: NStefan Hajnoczi <stefanha@redhat.com>
      c23d5f1d
    • G
      9pfs: local: open2: don't follow symlinks · a565fea5
      由 Greg Kurz 提交于 
      The local_open2() callback is vulnerable to symlink attacks because it
calls:

(1) open() which follows symbolic links for all path elements but the
    rightmost one
(2) local_set_xattr()->setxattr() which follows symbolic links for all
    path elements
(3) local_set_mapped_file_attr() which calls in turn local_fopen() and
    mkdir(), both functions following symbolic links for all path
    elements but the rightmost one
(4) local_post_create_passthrough() which calls in turn lchown() and
    chmod(), both functions also following symbolic links

This patch converts local_open2() to rely on opendir_nofollow() and
mkdirat() to fix (1), as well as local_set_xattrat(),
local_set_mapped_file_attrat() and local_set_cred_passthrough() to
fix (2), (3) and (4) respectively. Since local_open2() already opens
a descriptor to the target file, local_set_cred_passthrough() is
modified to reuse it instead of opening a new one.

The mapped and mapped-file security modes are supposed to be identical,
except for the place where credentials and file modes are stored. While
here, we also make that explicit by sharing the call to openat().

This partly fixes CVE-2016-9602.
Signed-off-by: NGreg Kurz <groug@kaod.org>
Reviewed-by: NStefan Hajnoczi <stefanha@redhat.com>
      a565fea5
    • G
      9pfs: local: mkdir: don't follow symlinks · 3f3a1699
      由 Greg Kurz 提交于 
      The local_mkdir() callback is vulnerable to symlink attacks because it
calls:

(1) mkdir() which follows symbolic links for all path elements but the
    rightmost one
(2) local_set_xattr()->setxattr() which follows symbolic links for all
    path elements
(3) local_set_mapped_file_attr() which calls in turn local_fopen() and
    mkdir(), both functions following symbolic links for all path
    elements but the rightmost one
(4) local_post_create_passthrough() which calls in turn lchown() and
    chmod(), both functions also following symbolic links

This patch converts local_mkdir() to rely on opendir_nofollow() and
mkdirat() to fix (1), as well as local_set_xattrat(),
local_set_mapped_file_attrat() and local_set_cred_passthrough() to
fix (2), (3) and (4) respectively.

The mapped and mapped-file security modes are supposed to be identical,
except for the place where credentials and file modes are stored. While
here, we also make that explicit by sharing the call to mkdirat().

This partly fixes CVE-2016-9602.
Signed-off-by: NGreg Kurz <groug@kaod.org>
Reviewed-by: NStefan Hajnoczi <stefanha@redhat.com>
      3f3a1699
    • G
      9pfs: local: mknod: don't follow symlinks · d815e721
      由 Greg Kurz 提交于 
      The local_mknod() callback is vulnerable to symlink attacks because it
calls:

(1) mknod() which follows symbolic links for all path elements but the
    rightmost one
(2) local_set_xattr()->setxattr() which follows symbolic links for all
    path elements
(3) local_set_mapped_file_attr() which calls in turn local_fopen() and
    mkdir(), both functions following symbolic links for all path
    elements but the rightmost one
(4) local_post_create_passthrough() which calls in turn lchown() and
    chmod(), both functions also following symbolic links

This patch converts local_mknod() to rely on opendir_nofollow() and
mknodat() to fix (1), as well as local_set_xattrat() and
local_set_mapped_file_attrat() to fix (2) and (3) respectively.

A new local_set_cred_passthrough() helper based on fchownat() and
fchmodat_nofollow() is introduced as a replacement to
local_post_create_passthrough() to fix (4).

The mapped and mapped-file security modes are supposed to be identical,
except for the place where credentials and file modes are stored. While
here, we also make that explicit by sharing the call to mknodat().

This partly fixes CVE-2016-9602.
Signed-off-by: NGreg Kurz <groug@kaod.org>
Reviewed-by: NStefan Hajnoczi <stefanha@redhat.com>
      d815e721
    • G
      9pfs: local: symlink: don't follow symlinks · 38771613
      由 Greg Kurz 提交于 
      The local_symlink() callback is vulnerable to symlink attacks because it
calls:

(1) symlink() which follows symbolic links for all path elements but the
    rightmost one
(2) open(O_NOFOLLOW) which follows symbolic links for all path elements but
    the rightmost one
(3) local_set_xattr()->setxattr() which follows symbolic links for all
    path elements
(4) local_set_mapped_file_attr() which calls in turn local_fopen() and
    mkdir(), both functions following symbolic links for all path
    elements but the rightmost one

This patch converts local_symlink() to rely on opendir_nofollow() and
symlinkat() to fix (1), openat(O_NOFOLLOW) to fix (2), as well as
local_set_xattrat() and local_set_mapped_file_attrat() to fix (3) and
(4) respectively.

This partly fixes CVE-2016-9602.
Signed-off-by: NGreg Kurz <groug@kaod.org>
Reviewed-by: NStefan Hajnoczi <stefanha@redhat.com>
      38771613
    • G
      9pfs: local: chown: don't follow symlinks · d369f207
      由 Greg Kurz 提交于 
      The local_chown() callback is vulnerable to symlink attacks because it
calls:

(1) lchown() which follows symbolic links for all path elements but the
    rightmost one
(2) local_set_xattr()->setxattr() which follows symbolic links for all
    path elements
(3) local_set_mapped_file_attr() which calls in turn local_fopen() and
    mkdir(), both functions following symbolic links for all path
    elements but the rightmost one

This patch converts local_chown() to rely on open_nofollow() and
fchownat() to fix (1), as well as local_set_xattrat() and
local_set_mapped_file_attrat() to fix (2) and (3) respectively.

This partly fixes CVE-2016-9602.
Signed-off-by: NGreg Kurz <groug@kaod.org>
Reviewed-by: NStefan Hajnoczi <stefanha@redhat.com>
      d369f207
    • G
      9pfs: local: chmod: don't follow symlinks · e3187a45
      由 Greg Kurz 提交于 
      The local_chmod() callback is vulnerable to symlink attacks because it
calls:

(1) chmod() which follows symbolic links for all path elements
(2) local_set_xattr()->setxattr() which follows symbolic links for all
    path elements
(3) local_set_mapped_file_attr() which calls in turn local_fopen() and
    mkdir(), both functions following symbolic links for all path
    elements but the rightmost one

We would need fchmodat() to implement AT_SYMLINK_NOFOLLOW to fix (1). This
isn't the case on linux unfortunately: the kernel doesn't even have a flags
argument to the syscall :-\ It is impossible to fix it in userspace in
a race-free manner. This patch hence converts local_chmod() to rely on
open_nofollow() and fchmod(). This fixes the vulnerability but introduces
a limitation: the target file must readable and/or writable for the call
to openat() to succeed.

It introduces a local_set_xattrat() replacement to local_set_xattr()
based on fsetxattrat() to fix (2), and a local_set_mapped_file_attrat()
replacement to local_set_mapped_file_attr() based on local_fopenat()
and mkdirat() to fix (3). No effort is made to factor out code because
both local_set_xattr() and local_set_mapped_file_attr() will be dropped
when all users have been converted to use the "at" versions.

This partly fixes CVE-2016-9602.
Signed-off-by: NGreg Kurz <groug@kaod.org>
Reviewed-by: NStefan Hajnoczi <stefanha@redhat.com>
      e3187a45
    • G
      9pfs: local: link: don't follow symlinks · ad0b46e6
      由 Greg Kurz 提交于 
      The local_link() callback is vulnerable to symlink attacks because it calls:

(1) link() which follows symbolic links for all path elements but the
    rightmost one
(2) local_create_mapped_attr_dir()->mkdir() which follows symbolic links
    for all path elements but the rightmost one

This patch converts local_link() to rely on opendir_nofollow() and linkat()
to fix (1), mkdirat() to fix (2).

This partly fixes CVE-2016-9602.
Signed-off-by: NGreg Kurz <groug@kaod.org>
Reviewed-by: NStefan Hajnoczi <stefanha@redhat.com>
      ad0b46e6