From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by passt.top (Postfix) with ESMTP id A95835A0082 for ; Tue, 14 Feb 2023 16:30:22 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1676388621; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=5c7djxCVzJFhp+GUJDJ9j0xYoEwCYRQERhQ+1hubXLQ=; b=RlSJva4DGliFQvqY7fLjcDRiLySPrm8HFtgiJ7rL1IZD1LBY87nO04A7KMuk1Yeqhfvqb2 6a1TkmwTYiurdVaOsVlDsCKXD/dI04X0Q+ReNciOiv2B46TB4vVoVw37jr/NxaMRTBni3p HKlk26NIHf9UCcapS0Cug1+Lv0YvboM= Received: from mail-ed1-f70.google.com (mail-ed1-f70.google.com [209.85.208.70]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-564-DK0TGGOLOV6FKPHYaO9nkw-1; Tue, 14 Feb 2023 10:30:19 -0500 X-MC-Unique: DK0TGGOLOV6FKPHYaO9nkw-1 Received: by mail-ed1-f70.google.com with SMTP id s21-20020a056402521500b004acd97105ffso2535760edd.19 for ; Tue, 14 Feb 2023 07:30:19 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=5c7djxCVzJFhp+GUJDJ9j0xYoEwCYRQERhQ+1hubXLQ=; b=lGEw2IEIF40StWJW79cOrY6NvevzBxPzWQplRWsvfXhcYcYbD4sB5F5UkDqVFr1oZn onxpmiyRuA7V/DPEXimR+7CrQFABgMrjN6HHqQMGpA6KQ3wSc7kZf2KdInqZyMjfZyEG Xo/x/tkqFIl4kS0WL8R7DZ64s9nvUyML6x7Jspmi5A/YNCRrt7fao4RFGd5m0u43hduA FEcBQv7lR5Jg4qgVP5LFPM8UJIuodbce7+6IhocDjVe8XsQCVctRR8urVFru0610pSBc pBYJptKbICducSernQ+wj/DAq9kQNKJ8Pfgw5k7Q1W04iN/GTN3VV6h1XtCh4NSVsZmo //HQ== X-Gm-Message-State: AO0yUKWJ97FoMh+/7UQ4gUOGFiN3F1I2YmWpl5EjeFmeU0w3QnEZwDr4 FNPiuEAxN9AFpqKHWZM7a5vxNCVnuf+6A1fVx7ukK1yHLD04llMb7vNochW3yaOp2HjZQRyZMYH k0GTfsHIDHLDD X-Received: by 2002:a17:906:12ca:b0:8b1:3d4:6a9d with SMTP id l10-20020a17090612ca00b008b103d46a9dmr3465185ejb.19.1676388618174; Tue, 14 Feb 2023 07:30:18 -0800 (PST) X-Google-Smtp-Source: AK7set+Uh58CaSxbeBrTNBknNLlePk80MGolkHus//BpAGpX7oyXak0u06XwdhleJTKpqFDprIAqIA== X-Received: by 2002:a17:906:12ca:b0:8b1:3d4:6a9d with SMTP id l10-20020a17090612ca00b008b103d46a9dmr3465159ejb.19.1676388617960; Tue, 14 Feb 2023 07:30:17 -0800 (PST) Received: from [10.43.2.39] (nat-pool-brq-t.redhat.com. [213.175.37.10]) by smtp.gmail.com with ESMTPSA id e13-20020a170906044d00b0088eb55ed9cbsm8376104eja.187.2023.02.14.07.30.17 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 14 Feb 2023 07:30:17 -0800 (PST) Message-ID: <56000087-8df0-294c-6f42-57983e15d383@redhat.com> Date: Tue, 14 Feb 2023 16:30:17 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.6.1 Subject: Re: [PATCH 4/4] qemu_passt: Don't let passt fork off To: Stefano Brivio References: <5abfc412e4692a38e980c8dc600e1bfbd03ddcfd.1676374699.git.mprivozn@redhat.com> <20230214140253.49bbc13a@elisabeth> From: =?UTF-8?B?TWljaGFsIFByw612b3puw61r?= In-Reply-To: <20230214140253.49bbc13a@elisabeth> X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-MailFrom: mprivozn@redhat.com X-Mailman-Rule-Hits: nonmember-moderation X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation Message-ID-Hash: PXI7K7SVOTVTR3U3ELW6N2CZ6IKLFPKR X-Message-ID-Hash: PXI7K7SVOTVTR3U3ELW6N2CZ6IKLFPKR X-Mailman-Approved-At: Wed, 15 Feb 2023 08:57:14 +0100 CC: libvir-list@redhat.com, passt-dev@passt.top X-Mailman-Version: 3.3.3 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On 2/14/23 14:02, Stefano Brivio wrote: > On Tue, 14 Feb 2023 12:51:22 +0100 > Michal Privoznik 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. That's not what's happening here. In fact, the opposite. We let libvirt create the file, lock and write it. Then the file's FD is leaked into passt process. This way, we can learn whether the PID file is still valid: if we just try to lock it and: a) fail, then the file is still locked, i.e. passt is still running, b) succeed, then the file is no longer locket by passt, i.e. it's no longer running. Thing is, passt doesn't even know about the FD (unless it starts closing FDs "randomly", e.g. via closefrom(3) or an alternative). This is the reason we can't use passt's --pid, because it writes the PID file without any locking dance. Therefore, libvirt can't know whether the PID file is still valid. If it did just read the file and killed PID from there it might kill a random process that was unfortunate to be assigned the same PID. BTW, there are two other issues with PID files in passt: 1) no locking means, that the file is very easily overwritten: $ passt --pid /tmp/passt.pid 2>/dev/null & \ passt --pid /tmp/passt.pid 2>/dev/null; \ pgrep passt ; cat /tmp/passt.pid [1] 21029 [1]+ Done passt --pid /tmp/passt.pid 2> /dev/null 21034 21035 21035 2) with --daemonize, only the PID of the parent process is written which is pretty much useless, as the parent quits shortly after clone(). Now, libvirt's virPidFile* machinery (src/util/virpidfile.c) ensures that case 1) won't happen (yes, we have internal APIs that read pid files unlocked, but those are not used from this code we're talking about). And this patch tries to fix the case 2) by instructing passt to not clone(). If it means that it can't use PID namespace, then so be it. It's better to not let processes behind. > >> 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. >> >> Fortunately, passt has '--foreground' argument, which causes it >> to undergo the same security measures but without forking off the >> child. > > 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. I understand that, but it's already confined in plenty other ways. It's way worse to leave a process running than being able to see other PIDs. > > 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... > Is there? If we have a helper process that fork()-s then the only way we can 'track' its PIDs (and kill them) is by placing them into a CGroup. Until we do that we have to trust helper processes to behave properly. But that's something I'd like to avoid. Michal