From: Laine Stump <laine@redhat.com>
To: Libvirt <libvir-list@redhat.com>, passt-dev@passt.top
Cc: Stefano Brivio <sbrivio@redhat.com>,
Michal Privoznik <mprivozn@redhat.com>
Subject: Re: [PATCH 4/4] qemu_passt: Don't let passt fork off
Date: Wed, 15 Feb 2023 02:50:59 -0500 [thread overview]
Message-ID: <90dbb5f3-7b3f-893c-ca32-a7653eb486c6@redhat.com> (raw)
In-Reply-To: <20230214140253.49bbc13a@elisabeth>
On 2/14/23 8:02 AM, Stefano Brivio wrote:
> On Tue, 14 Feb 2023 12:51:22 +0100
> Michal Privoznik <mprivozn@redhat.com> wrote:
>
>> When passt starts it tries to do some security measures to
>> restrict itself. For instance, it creates its own namespaces,
>> umounts basically everything, drops capabilities, forks off to
>> further restrict itself (the child is where all interesting work
>> takes place now). This is sound, except it's causing two
>> problems:
>>
>> 1) the PID file FD, which we leak into the passt process, gets
>> closed (and thus our virPidFile*() helpers see unlocked PID
>> file, which makes them think the process is gone),
>
> I didn't realise this was the case, but giving passt write (unless I'm
> missing something) access to a file created by libvirtd doesn't look
> desirable to me.
>
>> 2) the PID file no longer reflects true PID of the process.
>>
>> Worse, the child calls setsid() so we can't even kill the whole
>> process group. I mean, we can but it won't be any good.
I think that (incorrect PID in the pidfile) is happening because Michal
is using the original version of my patches that were pushed - I had
mimicked the behavior of slirp, where libvirt deamonizes the new
process. If that process then daemonizes itself, we have some sort of
"double daemon"; libvirt has saved off the pid of what it thinks is
going to be the final process, but then that process further forks and
exits from the process whose pid libvirt saved. But because passt was
cleaning up after itself I hadn't noticed the discrepancy in pids when
testing.
Without going into all the details of the pidfile and locking and etc, I
just want to say that if we can fork/exec dnsmasq and let it daemonize
itself and create its own pidfile, then certainly we can do the same
thing for passt. (and if there's a fundamental problem, then it's a
fundamental problem for dnsmasq as well).
>>
>> Fortunately, passt has '--foreground' argument, which causes it
>> to undergo the same security measures but without forking off the
>> child.
But if we do --foreground in combination with calling
virCommandDaemonize(), then won't we still have the problem that libvirt
won't know whether or not passt has failed to start (not unless we want
to put in a bunch of gorp to continue grabbing stderr while watching to
see if the passt process has exited, etc. It's going to take some
convincing for me to think we should run passt with --foreground rather
than letting it daemonize itself.
>
> They're not the same -- unfortunately they can't be, because, on Linux,
> you can't change the PID of an existing process, so there's no way to
> enter a new PID namespace without clone().
>
> If passt remains in the same PID namespace, it's still able to see PIDs
> of other processes, which is not desirable from a security perspective.
>
> Again from a security perspective, this is probably a small impact, so
> I guess it's fine if there's no other way around it. But I see a lot of
> ways around it...
>
next prev parent reply other threads:[~2023-02-15 7:51 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-02-14 11:51 [PATCH 0/4] qemu_passt: Don't let passt fork off Michal Privoznik
2023-02-14 11:51 ` [PATCH 1/4] Revert "qemu: allow passt to self-daemonize" Michal Privoznik
2023-02-15 7:06 ` Laine Stump
2023-02-14 11:51 ` [PATCH 2/4] qemu_extdevice: Make qemuExtDevicesHasDevice() check def->nets Michal Privoznik
2023-02-15 7:22 ` Laine Stump
2023-02-15 15:23 ` Michal Prívozník
2023-02-14 11:51 ` [PATCH 3/4] qemu_passt: Report error when getting passt PID failed Michal Privoznik
2023-02-15 7:24 ` Laine Stump
2023-02-14 11:51 ` [PATCH 4/4] qemu_passt: Don't let passt fork off Michal Privoznik
2023-02-14 13:02 ` Stefano Brivio
2023-02-14 15:30 ` Michal Prívozník
2023-02-14 16:22 ` Stefano Brivio
2023-02-15 7:50 ` Laine Stump [this message]
2023-02-15 17:04 ` Michal Prívozník
2023-02-15 18:22 ` Laine Stump
2023-02-15 18:30 ` Stefano Brivio
2023-02-16 8:52 ` Michal Prívozník
2023-02-16 9:07 ` Peter Krempa
2023-02-16 9:15 ` Stefano Brivio
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=90dbb5f3-7b3f-893c-ca32-a7653eb486c6@redhat.com \
--to=laine@redhat.com \
--cc=libvir-list@redhat.com \
--cc=mprivozn@redhat.com \
--cc=passt-dev@passt.top \
--cc=sbrivio@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://passt.top/passt
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for IMAP folder(s).