Re: [PATCH v11 7/8] pkcs7, x509: Add RSASSA-PSS support

[David Howells <dhowells@redhat.com>]

Ignat Korchagin <ignat@cloudflare.com> wrote:

> > +       case OID_id_rsassa_pss:
> > +               goto rsassa_pss;
> ...
> > +rsassa_pss:
> > +       if (!ctx->algo_params || !ctx->algo_params_size) {
> > +               pr_debug("RSASSA-PSS sig algo without parameters\n");
> > +               return -EBADMSG;
> > +       }
> > +
> > +       err = rsassa_parse_sig_params(sig, ctx->algo_params, ctx->algo_params_size);
> > +       if (err < 0)
> > +               return err;
> > +
> > +       sig->pkey_algo = "rsa";
> > +       sig->encoding = "emsa-pss";
> > +       goto out;
> >  }
> 
> I really don't like this. Is it possible to factor this out to a
> separate function and just call here? Should the factored function
> even be part of the implementation somehow?

I'll move the check into rsassa_parse_sig_params() and then move the remaining
code back into the switch case.  That also means that x509_note_sig_algo()
doesn't need the check either.  It does change the pr_fmt value seen by the
pr_debug(), but that's probably fine.

> >         ctx->last_oid = look_up_OID(value, vlen);
> >         if (ctx->last_oid == OID__NR) {
> > -               char buffer[50];
> > +               char buffer[56];
> >                 sprint_oid(value, vlen, buffer, sizeof(buffer));
> 
> I've seen this elsewhere in the crypto code (namely in ECC) but is it
> generally a good idea to declare long buffers on the stack?

It's not all that long (7 words on a 64-bit machine - similarish to a function
call), and the output of sprint_oid() is limited to it.

David