linux-kernel - Re: [PATCH v4 16/24] powerpc/pseries: Implement signed update for PLPKS objects

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <63aaa01d1649f2905b3bff8009bb9c3a47c82e50.camel@linux.ibm.com>
Date:   Mon, 30 Jan 2023 15:43:45 +1100
From:   Andrew Donnellan <ajd@...ux.ibm.com>
To:     Nicholas Piggin <npiggin@...il.com>, linuxppc-dev@...ts.ozlabs.org,
        linux-integrity@...r.kernel.org
Cc:     sudhakar@...ux.ibm.com, bgray@...ux.ibm.com, erichte@...ux.ibm.com,
        gregkh@...uxfoundation.org, nayna@...ux.ibm.com,
        linux-kernel@...r.kernel.org, zohar@...ux.ibm.com,
        gjoyce@...ux.ibm.com, ruscur@...sell.cc, gcwilson@...ux.ibm.com,
        joel@....id.au
Subject: Re: [PATCH v4 16/24] powerpc/pseries: Implement signed update for
 PLPKS objects

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)
> >                 err = -ENOENT;
> >                 break;
> >         case H_BUSY:
> > +       case H_LONG_BUSY_ORDER_1_MSEC:
> > +       case H_LONG_BUSY_ORDER_10_MSEC:
> > +       case H_LONG_BUSY_ORDER_100_MSEC:
> > +       case H_LONG_BUSY_ORDER_1_SEC:
> > +       case H_LONG_BUSY_ORDER_10_SEC:
> > +       case H_LONG_BUSY_ORDER_100_SEC:
> >                 err = -EBUSY;
> >                 break;
> >         case H_AUTHORITY:
> 
> 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.

Agreed - though we're not going to touch it in this series.

> 
> > @@ -184,14 +190,17 @@ static struct label *construct_label(char
> > *component, u8 varos, u8 *name,
> >                                      u16 namelen)
> >  {
> >         struct label *label;
> > -       size_t slen;
> > +       size_t slen = 0;
> >  
> >         if (!name || namelen > PLPKS_MAX_NAME_SIZE)
> >                 return ERR_PTR(-EINVAL);
> >  
> > -       slen = strlen(component);
> > -       if (component && slen > sizeof(label->attr.prefix))
> > -               return ERR_PTR(-EINVAL);
> > +       // Support NULL component for signed updates
> > +       if (component) {
> > +               slen = strlen(component);
> > +               if (slen > sizeof(label->attr.prefix))
> > +                       return ERR_PTR(-EINVAL);
> > +       }
> 
> Is this already a bug? Code checks for component != NULL but
> previously
> calls strlen which would oops on NULL component AFAIKS. Granted
> nothing
> is actually using any of this these days.

True, it should have been checking for NULL first, but as you say no-
one is using it.

> 
> 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).

I think the comment confuses more than it clarifies, I'll remove it.

As you say, read_var() should work fine with component == 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.

> > +EXPORT_SYMBOL(plpks_signed_update_var);
> 
> Sorry I missed it before -- can this be a _GPL export?

Indeed it should be - actually, I should check if I can get rid of the
export completely...

-- 
Andrew Donnellan    OzLabs, ADL Canberra
ajd@...ux.ibm.com   IBM Australia Limited