[PATCH] efi_loader: fix an IS_ERR() vs NULL check
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 dan.carpenter@linaro.org --- 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), &buf); - if (IS_ERR(capsule_sig)) { + if (!capsule_sig) { debug("Parsing variable's pkcs7 header failed\n"); - capsule_sig = NULL; goto out; }
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 dan.carpenter@linaro.org
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), &buf);
- 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;
Hi Dan, Heinrich
On Thu, 27 Jul 2023 at 11:25, Heinrich Schuchardt xypron.glpk@gmx.de 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 dan.carpenter@linaro.org
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), &buf);
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; }
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 xypron.glpk@gmx.de 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 dan.carpenter@linaro.org
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), &buf);
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
On Thu, 27 Jul 2023 at 12:24, Dan Carpenter dan.carpenter@linaro.org 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 xypron.glpk@gmx.de 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 dan.carpenter@linaro.org
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), &buf);
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 ilias.apalodimas@linaro.org
regards, 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 dan.carpenter@linaro.org
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), &buf);
- 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
participants (3)
-
Dan Carpenter -
Heinrich Schuchardt -
Ilias Apalodimas