| Message ID | 20200507001714.GA6487@nox.fritz.box |
|---|---|
| State | Accepted |
| Commit | 6f146155f879ff42d465f0cca8ec2a7f8cb0961e |
| Headers | show |
| Series | efi_loader: pkcs7_parse_message() returns error pointer | expand |
On 07.05.20 02:17, Patrick Wildt wrote: > Since pkcs7_parse_message() returns an error pointer, we must not > check for NULL. We have to explicitly set msg to NULL in the error > case, otherwise the call to pkcs7_free_message() on the goto err > path will assume it's a valid object. @Takahiro I think we should start documenting the library functions properly. The description in lib/crypto/pkcs7_parser.c does not conform to https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html#function-documentation Especially we should describe how errors are returned. Best regards Heinrich > > Signed-off-by: Patrick Wildt <patrick at blueri.se> > > diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c > index 5a9a6424cc..43a53d3dd1 100644 > --- a/lib/efi_loader/efi_image_loader.c > +++ b/lib/efi_loader/efi_image_loader.c > @@ -538,8 +538,9 @@ static bool efi_image_authenticate(void *efi, size_t efi_size) > } > msg = pkcs7_parse_message((void *)wincert + sizeof(*wincert), > wincert->dwLength - sizeof(*wincert)); > - if (!msg) { > + if (IS_ERR(msg)) { > debug("Parsing image's signature failed\n"); > + msg = NULL; > goto err; > } > >
On 07.05.20 02:17, Patrick Wildt wrote: > Since pkcs7_parse_message() returns an error pointer, we must not > check for NULL. We have to explicitly set msg to NULL in the error > case, otherwise the call to pkcs7_free_message() on the goto err > path will assume it's a valid object. > > Signed-off-by: Patrick Wildt <patrick at blueri.se> > > diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c > index 5a9a6424cc..43a53d3dd1 100644 > --- a/lib/efi_loader/efi_image_loader.c > +++ b/lib/efi_loader/efi_image_loader.c > @@ -538,8 +538,9 @@ static bool efi_image_authenticate(void *efi, size_t efi_size) > } > msg = pkcs7_parse_message((void *)wincert + sizeof(*wincert), > wincert->dwLength - sizeof(*wincert)); > - if (!msg) { > + if (IS_ERR(msg)) { Compiling with sandbox_defconfig results in: lib/efi_loader/efi_image_loader.c: In function ?efi_image_authenticate?: lib/efi_loader/efi_image_loader.c:541:7: warning: implicit declaration of function ?IS_ERR? [-Wimplicit-function-declaration] 541 | if (IS_ERR(msg)) { | ^~~~~~ I will add the missing #include <linux/err.h> when merging. Reviewed-by: Heinrich Schuchardt <xypron.glpk at gmx.de> > debug("Parsing image's signature failed\n"); > + msg = NULL; > goto err; > } > >
Heinrich, On Thu, May 07, 2020 at 04:47:22PM +0200, Heinrich Schuchardt wrote: > On 07.05.20 02:17, Patrick Wildt wrote: > > Since pkcs7_parse_message() returns an error pointer, we must not > > check for NULL. We have to explicitly set msg to NULL in the error > > case, otherwise the call to pkcs7_free_message() on the goto err > > path will assume it's a valid object. > > @Takahiro > I think we should start documenting the library functions properly. The Generally I agree, but > description in lib/crypto/pkcs7_parser.c does not conform to > https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html#function-documentation > Especially we should describe how errors are returned. Remember that this file, as well as others under lib/crypto, was imported from linux kernel source. I made a minimum set of changes to align it with U-Boot code. So I'm rather reluctant to modify the file. Thanks, -Takahiro Akashi > Best regards > > Heinrich > > > > > Signed-off-by: Patrick Wildt <patrick at blueri.se> > > > > diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c > > index 5a9a6424cc..43a53d3dd1 100644 > > --- a/lib/efi_loader/efi_image_loader.c > > +++ b/lib/efi_loader/efi_image_loader.c > > @@ -538,8 +538,9 @@ static bool efi_image_authenticate(void *efi, size_t efi_size) > > } > > msg = pkcs7_parse_message((void *)wincert + sizeof(*wincert), > > wincert->dwLength - sizeof(*wincert)); > > - if (!msg) { > > + if (IS_ERR(msg)) { > > debug("Parsing image's signature failed\n"); > > + msg = NULL; > > goto err; > > } > > > > >
diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c index 5a9a6424cc..43a53d3dd1 100644 --- a/lib/efi_loader/efi_image_loader.c +++ b/lib/efi_loader/efi_image_loader.c @@ -538,8 +538,9 @@ static bool efi_image_authenticate(void *efi, size_t efi_size) } msg = pkcs7_parse_message((void *)wincert + sizeof(*wincert), wincert->dwLength - sizeof(*wincert)); - if (!msg) { + if (IS_ERR(msg)) { debug("Parsing image's signature failed\n"); + msg = NULL; goto err; }
Since pkcs7_parse_message() returns an error pointer, we must not check for NULL. We have to explicitly set msg to NULL in the error case, otherwise the call to pkcs7_free_message() on the goto err path will assume it's a valid object. Signed-off-by: Patrick Wildt <patrick at blueri.se>