| Message ID | 20200507001318.GA6451@nox.fritz.box |
|---|---|
| State | Accepted |
| Commit | 9ad15227bb92acc2bf73c60da1bcf2ae3774246d |
| Headers | show |
| Series | efi_loader: efi_variable_parse_signature() returns NULL on error | expand |
On Thu, May 07, 2020 at 02:13:18AM +0200, Patrick Wildt wrote: > efi_variable_parse_signature() returns NULL on error, so IS_NULL() > is an incorrect check. The goto err leads to pkcs7_free_message(), > which works fine on a NULL ptr. > > Signed-off-by: Patrick Wildt <patrick at blueri.se> Reviewed-by: AKASHI Takahiro <takahiro.akashi at linaro.org> > > diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c > index 58f8fae358..c5fe896de2 100644 > --- a/lib/efi_loader/efi_variable.c > +++ b/lib/efi_loader/efi_variable.c > @@ -519,9 +519,8 @@ static efi_status_t efi_variable_authenticate(u16 *variable, > var_sig = efi_variable_parse_signature(auth->auth_info.cert_data, > auth->auth_info.hdr.dwLength > - sizeof(auth->auth_info)); > - if (IS_ERR(var_sig)) { > + if (!var_sig) { > debug("Parsing variable's signature failed\n"); > - var_sig = NULL; > goto err; > } >
On Thu, May 07, 2020 at 02:13:18AM +0200, Patrick Wildt wrote: > efi_variable_parse_signature() returns NULL on error, so IS_NULL() > is an incorrect check. The goto err leads to pkcs7_free_message(), > which works fine on a NULL ptr. > > Signed-off-by: Patrick Wildt <patrick at blueri.se> Reviewed-by: AKASHI Takahiro <takahiro.akashi at linaro.org> > > diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c > index 58f8fae358..c5fe896de2 100644 > --- a/lib/efi_loader/efi_variable.c > +++ b/lib/efi_loader/efi_variable.c > @@ -519,9 +519,8 @@ static efi_status_t efi_variable_authenticate(u16 *variable, > var_sig = efi_variable_parse_signature(auth->auth_info.cert_data, > auth->auth_info.hdr.dwLength > - sizeof(auth->auth_info)); > - if (IS_ERR(var_sig)) { > + if (!var_sig) { > debug("Parsing variable's signature failed\n"); > - var_sig = NULL; > goto err; > } >
On 07.05.20 09:15, AKASHI Takahiro wrote: > On Thu, May 07, 2020 at 02:13:18AM +0200, Patrick Wildt wrote: >> efi_variable_parse_signature() returns NULL on error, so IS_NULL() IS_NULL() seems to be a typo. I will replace it by IS_ERR() when merging. >> is an incorrect check. The goto err leads to pkcs7_free_message(), >> which works fine on a NULL ptr. >> >> Signed-off-by: Patrick Wildt <patrick at blueri.se> > > Reviewed-by: AKASHI Takahiro <takahiro.akashi at linaro.org> Hello Takahiro, there is a second instance of the same error: lib/efi_loader/efi_variable.c:412: ??????? msg = pkcs7_parse_message(ebuf, ebuflen); ??????? free(ebuf); out: ????????if (IS_ERR(msg)) ????????????????return NULL; Please, send a patch for that one too. Best regards Heinrich > >> >> diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c >> index 58f8fae358..c5fe896de2 100644 >> --- a/lib/efi_loader/efi_variable.c >> +++ b/lib/efi_loader/efi_variable.c >> @@ -519,9 +519,8 @@ static efi_status_t efi_variable_authenticate(u16 *variable, >> var_sig = efi_variable_parse_signature(auth->auth_info.cert_data, >> auth->auth_info.hdr.dwLength >> - sizeof(auth->auth_info)); >> - if (IS_ERR(var_sig)) { >> + if (!var_sig) { >> debug("Parsing variable's signature failed\n"); >> - var_sig = NULL; >> goto err; >> } >>
On Thu, May 07, 2020 at 03:53:29PM +0200, Heinrich Schuchardt wrote: > On 07.05.20 09:15, AKASHI Takahiro wrote: > > On Thu, May 07, 2020 at 02:13:18AM +0200, Patrick Wildt wrote: > >> efi_variable_parse_signature() returns NULL on error, so IS_NULL() > > IS_NULL() seems to be a typo. I will replace it by IS_ERR() when merging. Hi, Yes, sorry. > >> is an incorrect check. The goto err leads to pkcs7_free_message(), > >> which works fine on a NULL ptr. > >> > >> Signed-off-by: Patrick Wildt <patrick at blueri.se> > > > > Reviewed-by: AKASHI Takahiro <takahiro.akashi at linaro.org> > > Hello Takahiro, > > there is a second instance of the same error: > > lib/efi_loader/efi_variable.c:412: > > ??????? msg = pkcs7_parse_message(ebuf, ebuflen); > ??????? free(ebuf); > out: > ????????if (IS_ERR(msg)) > ????????????????return NULL; > > Please, send a patch for that one too. > > Best regards > > Heinrich No, that is ok. pkcs7_parse_message() returns an ERR_PTR() on failure, as you saw in my other diff. That's why IS_ERR(msg) is fine here, and we can return NULL. Best regards, Patrick > > > >> > >> diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c > >> index 58f8fae358..c5fe896de2 100644 > >> --- a/lib/efi_loader/efi_variable.c > >> +++ b/lib/efi_loader/efi_variable.c > >> @@ -519,9 +519,8 @@ static efi_status_t efi_variable_authenticate(u16 *variable, > >> var_sig = efi_variable_parse_signature(auth->auth_info.cert_data, > >> auth->auth_info.hdr.dwLength > >> - sizeof(auth->auth_info)); > >> - if (IS_ERR(var_sig)) { > >> + if (!var_sig) { > >> debug("Parsing variable's signature failed\n"); > >> - var_sig = NULL; > >> goto err; > >> } > >> >
diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c index 58f8fae358..c5fe896de2 100644 --- a/lib/efi_loader/efi_variable.c +++ b/lib/efi_loader/efi_variable.c @@ -519,9 +519,8 @@ static efi_status_t efi_variable_authenticate(u16 *variable, var_sig = efi_variable_parse_signature(auth->auth_info.cert_data, auth->auth_info.hdr.dwLength - sizeof(auth->auth_info)); - if (IS_ERR(var_sig)) { + if (!var_sig) { debug("Parsing variable's signature failed\n"); - var_sig = NULL; goto err; }
efi_variable_parse_signature() returns NULL on error, so IS_NULL() is an incorrect check. The goto err leads to pkcs7_free_message(), which works fine on a NULL ptr. Signed-off-by: Patrick Wildt <patrick at blueri.se>