Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Date:   Wed, 18 May 2022 08:13:35 -0700
From:   Matthias Kaehlcke <mka@chromium.org>
To:     Milan Broz <gmazyland@gmail.com>
Cc:     Alasdair Kergon <agk@redhat.com>,
        Mike Snitzer <snitzer@kernel.org>,
        Kees Cook <keescook@chromium.org>,
        James Morris <jmorris@namei.org>,
        "Serge E . Hallyn" <serge@hallyn.com>, dm-devel@redhat.com,
        Douglas Anderson <dianders@chromium.org>,
        linux-kernel@vger.kernel.org, linux-raid@vger.kernel.org,
        Song Liu <song@kernel.org>,
        linux-security-module@vger.kernel.org
Subject: Re: [dm-devel] [PATCH v4 1/3] dm: Add verity helpers for LoadPin
Message-ID: <YoUNH7MrfEb844ft@google.com>
References: <20220517233457.1123309-1-mka@chromium.org>
 <20220517163437.v4.1.I3e928575a23481121e73286874c4c2bdb403355d@changeid>
 <19149028-ec94-8f64-aed4-1e58f29942a8@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <19149028-ec94-8f64-aed4-1e58f29942a8@gmail.com>
Precedence: bulk

Hi Milan,

On Wed, May 18, 2022 at 09:57:43AM +0200, Milan Broz wrote:
> On 18/05/2022 01:34, Matthias Kaehlcke wrote:
> > LoadPin limits loading of kernel modules, firmware and certain
> > other files to a 'pinned' file system (typically a read-only
> > rootfs). To provide more flexibility LoadPin is being extended
> > to also allow loading these files from trusted dm-verity
> > devices. For that purpose LoadPin can be provided with a list
> > of verity root digests that it should consider as trusted.
> > 
> > Add a bunch of helpers to allow LoadPin to check whether a DM
> > device is a trusted verity device. The new functions broadly
> > fall in two categories: those that need access to verity
> > internals (like the root digest), and the 'glue' between
> > LoadPin and verity. The new file dm-verity-loadpin.c contains
> > the glue functions.
> > 
> > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> 
> ...
> 
> > +
> > +	if (dm_verity_get_root_digest(ti, &root_digest, &digest_size))
> > +		return false;
> 
> Almost unrelated note, but as there are more and more situations
> that checks verity root digest, shouldn't we export this as read-only
> sysfs attribute for DM verity devices?
> 
> Attacker can always calculate (but not change) Merkle tree, so this
> is not something that need to be hidden.
> 
> It would allow userspace to easily enumerate trusted DM devices without
> calling kernel ioctls...

I guess that's an option if there are scenarios where it is useful. It
should probably be a separate patch, since it isn't directly related with
extending LoadPin support to trusted verity devices.

> > +
> > +	table = dm_get_live_table(md, &srcu_idx);
> > +
> > +	if (dm_table_get_num_targets(table) != 1)
> > +		goto out;
> > +
> > +	ti = dm_table_get_target(table, 0);
> > +
> > +	if (is_trusted_verity_target(ti))
> > +		trusted = true;
> 
> What happens is someone reloads verity table later with
> a different content (or even different target type)?
> Does LoadPin even care here?

LoadPin cares, but only when new kernel files are loaded. It will then check
against the new verity table, and only allow loading of the file if it comes
from a verity target with a trusted digest.

> >   static struct target_type verity_target = {
> >   	.name		= "verity",
> >   	.version	= {1, 8, 0},
> 
> Please increase the minor version, it is very useful to detect (in logs)
> that the target driver has compatible extensions.

I can do that, but would like to confirm that this is really needed/desired.
This patch adds kernel-internal APIs which aren't accessible to userspace,
that don't impact verity directly, so I'm not sure an increased minor version
would be useful.

> I guess this change does not affect userspace veristysetup
> (as it is used handled by different tooling), right?

Correct, from the verity side this is effectively a NOP, the new
kernel-internal APIs only provide information to LoadPin, but don't
change any verity specific behavior.