Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 14 Jul 2023 16:56:56 -0700
From:   Fan Wu <wufan@linux.microsoft.com>
To:     Paul Moore <paul@paul-moore.com>
Cc:     corbet@lwn.net, zohar@linux.ibm.com, jmorris@namei.org,
        serge@hallyn.com, tytso@mit.edu, ebiggers@kernel.org,
        axboe@kernel.dk, agk@redhat.com, snitzer@kernel.org,
        eparis@redhat.com, linux-doc@vger.kernel.org,
        linux-integrity@vger.kernel.org,
        linux-security-module@vger.kernel.org,
        linux-fscrypt@vger.kernel.org, linux-block@vger.kernel.org,
        dm-devel@redhat.com, audit@vger.kernel.org,
        roberto.sassu@huawei.com, linux-kernel@vger.kernel.org,
        Deven Bowers <deven.desai@linux.microsoft.com>
Subject: Re: [PATCH RFC v10 5/17] ipe: introduce 'boot_verified' as a trust
  provider
Message-ID: <20230714235656.GD15267@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net>
References: <1687986571-16823-6-git-send-email-wufan@linux.microsoft.com>
 <7b0f16fd49fb3490af1018eba986d0e4.paul@paul-moore.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <7b0f16fd49fb3490af1018eba986d0e4.paul@paul-moore.com>
User-Agent: Mutt/1.5.21 (2010-09-15)
Precedence: bulk

On Sat, Jul 08, 2023 at 12:23:02AM -0400, Paul Moore wrote:
> On Jun 28, 2023 Fan Wu <wufan@linux.microsoft.com> wrote:
> > 
> > IPE is designed to provide system level trust guarantees, this usually
> > implies that trust starts from bootup with a hardware root of trust,
> > which validates the bootloader. After this, the bootloader verifies the
> > kernel and the initramfs.
> > 
> > As there's no currently supported integrity method for initramfs, and
> > it's typically already verified by the bootloader, introduce a property
> > that causes the first superblock to have an execution to be "pinned",
> > which is typically initramfs.
> > 
> > When the "pinned" device is unmounted, it will be "unpinned" and
> > `boot_verified` property will always evaluate to false afterward.
> > 
> > We use a pointer with a spin_lock to "pin" the device instead of rcu
> > because rcu synchronization may sleep, which is not allowed when
> > unmounting a device.
> > 
> > Signed-off-by: Deven Bowers <deven.desai@linux.microsoft.com>
> > Signed-off-by: Fan Wu <wufan@linux.microsoft.com>
> > ---
> >  security/ipe/eval.c          | 72 +++++++++++++++++++++++++++++++++++-
> >  security/ipe/eval.h          |  2 +
> >  security/ipe/hooks.c         | 12 ++++++
> >  security/ipe/hooks.h         |  2 +
> >  security/ipe/ipe.c           |  1 +
> >  security/ipe/policy.h        |  2 +
> >  security/ipe/policy_parser.c | 37 +++++++++++++++++-
> >  7 files changed, 126 insertions(+), 2 deletions(-)
> 
> The compilation errors continue into this patch.
> 
Sorry again for the header file problem.

> > diff --git a/security/ipe/policy_parser.c b/security/ipe/policy_parser.c
> > index 27e5767480b0..28c14adfe6d2 100644
> > --- a/security/ipe/policy_parser.c
> > +++ b/security/ipe/policy_parser.c
> > @@ -265,6 +265,12 @@ static enum ipe_action_type parse_action(char *t)
> >  	return match_token(t, action_tokens, args);
> >  }
> >  
> > +static const match_table_t property_tokens = {
> > +	{__IPE_PROP_BOOT_VERIFIED_FALSE,	"boot_verified=FALSE"},
> > +	{__IPE_PROP_BOOT_VERIFIED_TRUE,		"boot_verified=TRUE"},
> > +	{__IPE_PROP_INVALID,			NULL}
> > +};
> > +
> >  /**
> >   * parse_property - Parse the property type given a token string.
> >   * @t: Supplies the token string to be parsed.
> > @@ -277,7 +283,36 @@ static enum ipe_action_type parse_action(char *t)
> >   */
> >  static int parse_property(char *t, struct ipe_rule *r)
> >  {
> > -	return -EBADMSG;
> > +	substring_t args[MAX_OPT_ARGS];
> > +	struct ipe_prop *p = NULL;
> > +	int rc = 0;
> > +	int token;
> > +
> > +	p = kzalloc(sizeof(*p), GFP_KERNEL);
> > +	if (!p)
> > +		return -ENOMEM;
> > +
> > +	token = match_token(t, property_tokens, args);
> > +
> > +	switch (token) {
> > +	case __IPE_PROP_BOOT_VERIFIED_FALSE:
> > +	case __IPE_PROP_BOOT_VERIFIED_TRUE:
> > +		p->type = token;
> > +		break;
> > +	case __IPE_PROP_INVALID:
> 
> You generally don't need to explicitly specify a case if the code
> immediately falls through to 'default'.
> 
Got it, I will remove this line.

> > +	default:
> > +		rc = -EBADMSG;
> > +		break;
> > +	}
> > +	if (rc)
> > +		goto err;
> > +	list_add_tail(&p->next, &r->props);
> > +
> > +out:
> > +	return rc;
> > +err:
> > +	kfree(p);
> > +	goto out;
> 
> Once again, don't use a goto when the jump destination simply does a
> return, do the return directly.
> 
Sure, I will replace the goto with return.

-Fan
> >  }
> >  
> >  /**
> > -- 
> > 2.25.1
> 
> --
> paul-moore.com