Feedback-ID: i4421424f:Fastmail
Message-ID: <658d61ef8b6b6bb45fa946283b04c75dfac7a749.camel@russell.cc>
Subject: Re: [PATCH v4 16/24] powerpc/pseries: Implement signed update for
 PLPKS objects
From:   Russell Currey <ruscur@russell.cc>
To:     Andrew Donnellan <ajd@linux.ibm.com>,
        Nicholas Piggin <npiggin@gmail.com>,
        linuxppc-dev@lists.ozlabs.org, linux-integrity@vger.kernel.org
Cc:     sudhakar@linux.ibm.com, bgray@linux.ibm.com, erichte@linux.ibm.com,
        gregkh@linuxfoundation.org, nayna@linux.ibm.com,
        linux-kernel@vger.kernel.org, zohar@linux.ibm.com,
        gjoyce@linux.ibm.com, gcwilson@linux.ibm.com, joel@jms.id.au
Date:   Tue, 31 Jan 2023 15:23:52 +1100
In-Reply-To: <63aaa01d1649f2905b3bff8009bb9c3a47c82e50.camel@linux.ibm.com>
References: <20230120074306.1326298-1-ajd@linux.ibm.com>
         <20230120074306.1326298-17-ajd@linux.ibm.com>
         <CQ04OOT6CW1A.MCLZN2B4BTWK@bobo>
         <63aaa01d1649f2905b3bff8009bb9c3a47c82e50.camel@linux.ibm.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.46.3 (3.46.3-1.fc37) 
MIME-Version: 1.0
Precedence: bulk

On Mon, 2023-01-30 at 15:43 +1100, Andrew Donnellan wrote:
> On Tue, 2023-01-24 at 14:16 +1000, Nicholas Piggin wrote:
> > > diff --git a/arch/powerpc/platforms/pseries/plpks.c
> > > b/arch/powerpc/platforms/pseries/plpks.c
> > > index 1189246b03dc..796ed5544ee5 100644
> > > --- a/arch/powerpc/platforms/pseries/plpks.c
> > > +++ b/arch/powerpc/platforms/pseries/plpks.c
> > > @@ -81,6 +81,12 @@ static int pseries_status_to_err(int rc)
> > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0err =3D -ENOENT;
> > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0break;
> > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0case H_BUSY:
> > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0case H_LONG_BUSY_ORDER_1_M=
SEC:
> > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0case H_LONG_BUSY_ORDER_10_=
MSEC:
> > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0case H_LONG_BUSY_ORDER_100=
_MSEC:
> > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0case H_LONG_BUSY_ORDER_1_S=
EC:
> > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0case H_LONG_BUSY_ORDER_10_=
SEC:
> > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0case H_LONG_BUSY_ORDER_100=
_SEC:
> > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0err =3D -EBUSY;
> > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0break;
> > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0case H_AUTHORITY:
> >=20
> > This is a bit sad to maintain here. It's duplicating bits with
> > hvcs_convert, and a bunch of open coded places. Probably not the
> > series to do anything about. Would be nice if we could standardise
> > it though.
>=20
> Agreed - though we're not going to touch it in this series.
>=20
> >=20
> > > @@ -184,14 +190,17 @@ static struct label *construct_label(char
> > > *component, u8 varos, u8 *name,
> > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 u1=
6 namelen)
> > > =C2=A0{
> > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0struct label *label;
> > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0size_t slen;
> > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0size_t slen =3D 0;
> > > =C2=A0
> > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if (!name || namelen =
> PLPKS_MAX_NAME_SIZE)
> > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0return ERR_PTR(-EINVAL);
> > > =C2=A0
> > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0slen =3D strlen(component)=
;
> > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if (component && slen > si=
zeof(label->attr.prefix))
> > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0return ERR_PTR(-EINVAL);
> > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0// Support NULL component =
for signed updates
> > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if (component) {
> > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0slen =3D strlen(component);
> > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0if (slen > sizeof(label->attr.prefix))
> > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0ret=
urn ERR_PTR(-EINVAL);
> > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0}
> >=20
> > Is this already a bug? Code checks for component !=3D NULL but
> > previously
> > calls strlen which would oops on NULL component AFAIKS. Granted
> > nothing
> > is actually using any of this these days.
>=20
> True, it should have been checking for NULL first, but as you say no-
> one is using it.
>=20
> >=20
> > It already seems like it's supposed to be allowed to rad NULL
> > component
> > with read_var though? Why the differences, why not always allow
> > NULL
> > component? (I assume there is some reason, I just don't know
> > anything
> > about secvar or secure boot).
>=20
> I think the comment confuses more than it clarifies, I'll remove it.
>=20
> As you say, read_var() should work fine with component =3D=3D NULL,
> though
> write_var() checks it. The only rule I can find in the spec is that
> signed update calls *must* set the component to NULL. I'm seeking
> clarification on that.

Signed update calls *must* set the component to NULL.

We could just call construct_label() with NULL as the component
directly but I think it's better to explicitly check var->component and
return so the caller knows what they're trying to do is wrong.

>=20
> > > +EXPORT_SYMBOL(plpks_signed_update_var);
> >=20
> > Sorry I missed it before -- can this be a _GPL export?
>=20
> Indeed it should be - actually, I should check if I can get rid of
> the
> export completely...
>=20