Re: [PATCH] efi_loader: fix an IS_ERR() vs NULL check

2023-07-27 Thread Dan Carpenter 
On Thu, Jul 27, 2023 at 11:28:52AM +0300, Ilias Apalodimas wrote:
> Hi Dan, Heinrich
> 
> On Thu, 27 Jul 2023 at 11:25, Heinrich Schuchardt  wrote:
> >
> > On 7/27/23 09:16, Dan Carpenter wrote:
> > > The efi_parse_pkcs7_header() function returns NULL on error so the check
> > > for IS_ERR() should be changed to a NULL check.
> > >
> > > Signed-off-by: Dan Carpenter 
> > > ---
> > >   lib/efi_loader/efi_capsule.c | 3 +--
> > >   1 file changed, 1 insertion(+), 2 deletions(-)
> > >
> > > diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c
> > > index 7a6f195cbc02..c98cff812f10 100644
> > > --- a/lib/efi_loader/efi_capsule.c
> > > +++ b/lib/efi_loader/efi_capsule.c
> > > @@ -368,9 +368,8 @@ efi_status_t efi_capsule_authenticate(const void 
> > > *capsule, efi_uintn_t capsule_s
> > >
> > > auth_hdr->auth_info.hdr.dwLength
> > >- sizeof(auth_hdr->auth_info),
> > >);
> > > - if (IS_ERR(capsule_sig)) {
> > > + if (!capsule_sig) {
> >
> > Thanks for addressing this bug.
> >
> > Can we get rid of all the IS_ERR() in efi_capsule_authenticate() and use
> > IS_ERR_OR_NULL() here?
> >
> > Best regards
> 
> Heinrich is right here.   This seems more fundamentally broken as
> efi_parse_pkcs7_header() can return an IS_ERR() or NULL...

No, it can only return NULL.  That's also how the function is
documented.

 * Return:  Pointer to pkcs7_message structure on success, NULL on error

regards,
dan carpenter

Re: [PATCH] efi_loader: fix an IS_ERR() vs NULL check

2023-07-27 Thread Dan Carpenter 
On Thu, Jul 27, 2023 at 10:25:55AM +0200, Heinrich Schuchardt wrote:
> On 7/27/23 09:16, Dan Carpenter wrote:
> > The efi_parse_pkcs7_header() function returns NULL on error so the check
> > for IS_ERR() should be changed to a NULL check.
> > 
> > Signed-off-by: Dan Carpenter 
> > ---
> >   lib/efi_loader/efi_capsule.c | 3 +--
> >   1 file changed, 1 insertion(+), 2 deletions(-)
> > 
> > diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c
> > index 7a6f195cbc02..c98cff812f10 100644
> > --- a/lib/efi_loader/efi_capsule.c
> > +++ b/lib/efi_loader/efi_capsule.c
> > @@ -368,9 +368,8 @@ efi_status_t efi_capsule_authenticate(const void 
> > *capsule, efi_uintn_t capsule_s
> >  auth_hdr->auth_info.hdr.dwLength
> >  - sizeof(auth_hdr->auth_info),
> >  );
> > -   if (IS_ERR(capsule_sig)) {
> > +   if (!capsule_sig) {
> 
> Thanks for addressing this bug.
> 
> Can we get rid of all the IS_ERR() in efi_capsule_authenticate() and use
> IS_ERR_OR_NULL() here?

Ugh...  Please don't do that.  In u-boot it probably doesn't matter but
in the kernel they mean very different things.  A NULL in that context
means the feature is optional and disabled deliberately but an error
pointer means there was an error.

int blink_LEDS()
{
led = get_leds();
if (IS_ERR_OR_NULL(led)) {
if (IS_ERR(led))
print("LEDs are dead\n");
return PTR_ERR(led); <-- returns success if led is NULL
}

return led->blink();
}

So checking efi_capsule_authenticate() for IS_ERR_OR_NULL() means it's
optional and more options means more complications.

regards,
dan carpenter

[PATCH] efi_loader: fix an IS_ERR() vs NULL check

2023-07-27 Thread Dan Carpenter 
The efi_parse_pkcs7_header() function returns NULL on error so the check
for IS_ERR() should be changed to a NULL check.

Signed-off-by: Dan Carpenter 
---
 lib/efi_loader/efi_capsule.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c
index 7a6f195cbc02..c98cff812f10 100644
--- a/lib/efi_loader/efi_capsule.c
+++ b/lib/efi_loader/efi_capsule.c
@@ -368,9 +368,8 @@ efi_status_t efi_capsule_authenticate(const void *capsule, 
efi_uintn_t capsule_s
 auth_hdr->auth_info.hdr.dwLength
 - sizeof(auth_hdr->auth_info),
 );
-   if (IS_ERR(capsule_sig)) {
+   if (!capsule_sig) {
debug("Parsing variable's pkcs7 header failed\n");
-   capsule_sig = NULL;
goto out;
}
 
-- 
2.39.2

Re: [PATCH] efi_loader: fix an IS_ERR() vs NULL check

2023-07-27 Thread Ilias Apalodimas 
On Thu, 27 Jul 2023 at 12:24, Dan Carpenter  wrote:
>
> On Thu, Jul 27, 2023 at 11:28:52AM +0300, Ilias Apalodimas wrote:
> > Hi Dan, Heinrich
> >
> > On Thu, 27 Jul 2023 at 11:25, Heinrich Schuchardt  
> > wrote:
> > >
> > > On 7/27/23 09:16, Dan Carpenter wrote:
> > > > The efi_parse_pkcs7_header() function returns NULL on error so the check
> > > > for IS_ERR() should be changed to a NULL check.
> > > >
> > > > Signed-off-by: Dan Carpenter 
> > > > ---
> > > >   lib/efi_loader/efi_capsule.c | 3 +--
> > > >   1 file changed, 1 insertion(+), 2 deletions(-)
> > > >
> > > > diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c
> > > > index 7a6f195cbc02..c98cff812f10 100644
> > > > --- a/lib/efi_loader/efi_capsule.c
> > > > +++ b/lib/efi_loader/efi_capsule.c
> > > > @@ -368,9 +368,8 @@ efi_status_t efi_capsule_authenticate(const void 
> > > > *capsule, efi_uintn_t capsule_s
> > > >
> > > > auth_hdr->auth_info.hdr.dwLength
> > > >- 
> > > > sizeof(auth_hdr->auth_info),
> > > >);
> > > > - if (IS_ERR(capsule_sig)) {
> > > > + if (!capsule_sig) {
> > >
> > > Thanks for addressing this bug.
> > >
> > > Can we get rid of all the IS_ERR() in efi_capsule_authenticate() and use
> > > IS_ERR_OR_NULL() here?
> > >
> > > Best regards
> >
> > Heinrich is right here.   This seems more fundamentally broken as
> > efi_parse_pkcs7_header() can return an IS_ERR() or NULL...
>
> No, it can only return NULL.  That's also how the function is
> documented.
>
>  * Return:  Pointer to pkcs7_message structure on success, NULL on error

Ah you are right, I misread pkcs7_parse_message()

Reviewed-by: Ilias Apalodimas 

>
> regards,
> dan carpenter
>

Re: [PATCH] efi_loader: fix an IS_ERR() vs NULL check

2023-07-27 Thread Ilias Apalodimas 
Hi Dan, Heinrich

On Thu, 27 Jul 2023 at 11:25, Heinrich Schuchardt  wrote:
>
> On 7/27/23 09:16, Dan Carpenter wrote:
> > The efi_parse_pkcs7_header() function returns NULL on error so the check
> > for IS_ERR() should be changed to a NULL check.
> >
> > Signed-off-by: Dan Carpenter 
> > ---
> >   lib/efi_loader/efi_capsule.c | 3 +--
> >   1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c
> > index 7a6f195cbc02..c98cff812f10 100644
> > --- a/lib/efi_loader/efi_capsule.c
> > +++ b/lib/efi_loader/efi_capsule.c
> > @@ -368,9 +368,8 @@ efi_status_t efi_capsule_authenticate(const void 
> > *capsule, efi_uintn_t capsule_s
> >auth_hdr->auth_info.hdr.dwLength
> >- sizeof(auth_hdr->auth_info),
> >);
> > - if (IS_ERR(capsule_sig)) {
> > + if (!capsule_sig) {
>
> Thanks for addressing this bug.
>
> Can we get rid of all the IS_ERR() in efi_capsule_authenticate() and use
> IS_ERR_OR_NULL() here?
>
> Best regards

Heinrich is right here.   This seems more fundamentally broken as
efi_parse_pkcs7_header() can return an IS_ERR() or NULL...

Thanks
/Ilias
>
> Heinrich
>
> >   debug("Parsing variable's pkcs7 header failed\n");
> > - capsule_sig = NULL;
> >   goto out;
> >   }
> >
>

Re: [PATCH] efi_loader: fix an IS_ERR() vs NULL check

2023-07-27 Thread Heinrich Schuchardt 

On 7/27/23 09:16, Dan Carpenter wrote:

The efi_parse_pkcs7_header() function returns NULL on error so the check
for IS_ERR() should be changed to a NULL check.

Signed-off-by: Dan Carpenter 
---
  lib/efi_loader/efi_capsule.c | 3 +--
  1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c
index 7a6f195cbc02..c98cff812f10 100644
--- a/lib/efi_loader/efi_capsule.c
+++ b/lib/efi_loader/efi_capsule.c
@@ -368,9 +368,8 @@ efi_status_t efi_capsule_authenticate(const void *capsule, 
efi_uintn_t capsule_s
 auth_hdr->auth_info.hdr.dwLength
 - sizeof(auth_hdr->auth_info),
 );
-   if (IS_ERR(capsule_sig)) {
+   if (!capsule_sig) {


Thanks for addressing this bug.

Can we get rid of all the IS_ERR() in efi_capsule_authenticate() and use
IS_ERR_OR_NULL() here?

Best regards

Heinrich


debug("Parsing variable's pkcs7 header failed\n");
-   capsule_sig = NULL;
goto out;
}