Hi, On Mon, Feb 26, 2024 at 12:31:55AM +0800, Mr-Mr-key wrote: > Our team found that a public QEMU's 9pfs security issue[1] also exists in > upstream kvmtool's 9pfs device. A privileged guest user can create and > access the special device file(e.g., block files) in the shared folder, > allowing the malicious user to access the host device and acheive > privileged escalation. And I have sent the reproduction steps to Will. > [1] https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2023-2861 > > Root cause && fix suggestions： > The virtio_p9_open function code on the 9p.c only checks file directory attributes, but does not check special files. > Special device files can be filtered on the device through the S_IFREG and > S_IFDIR flag bits. A possible patch is as follows, and I have verified > that it does make a difference. Missing Signed-off-by line. > ...n-kernel-irqchip-before-creating-PIT.patch | 45 +++++++++++++++++++ > virtio/9p.c | 15 ++++++- > 2 files changed, 59 insertions(+), 1 deletion(-) > create mode 100644 0001-x86-Enable-in-kernel-irqchip-before-creating-PIT.patch (aside: I think you've accidentally included another patch here) > diff --git a/virtio/9p.c b/virtio/9p.c > index 2fa6f28..902da90 100644 > --- a/virtio/9p.c > +++ b/virtio/9p.c > @@ -221,6 +221,15 @@ static bool is_dir(struct p9_fid *fid) > return S_ISDIR(st.st_mode); > } > > +static bool is_reg(struct p9_fid *fid) > +{ > + struct stat st; > + > + stat(fid->abs_path, &st); > + > + return S_ISREG(st.st_mode); > +} > + > /* path is always absolute */ > static bool path_is_illegal(const char *path) > { > @@ -290,7 +299,11 @@ static void virtio_p9_open(struct p9_dev *p9dev, > goto err_out; > > stat2qid(&st, &qid); > - > + > + if (!is_dir(new_fid) && !is_reg(new_fid)){ > + goto err_out; > + } We already check is_dir() immediately below, so I think you can rewrite this as: if (is_dir(new_fid)) { ... } else if (is_reg(new_fid)) { ... } else { goto err_out; } I was also wondering whether we care about symlinks, but I couldn't get S_ISLNK to do anything useful in my local testing as I think stat() is always following them. So that should mean that we're ok. Will
