| Message ID | 20180325004818.13724-2-npiggin@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | NULL pointer dereferences | expand |
On Sun, Mar 25, 2018 at 11:48 AM, Nicholas Piggin <npiggin@gmail.com> wrote: > If nvram_check fails, leaving skiboot_part_hdr == NULL, do not > have nvram_query proceed using that pointer, just return NULL. Shouldn't the checking in nvram_validate() catch that case? If not we should probably fix it there. > Signed-off-by: Nicholas Piggin <npiggin@gmail.com> > --- > core/nvram-format.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/core/nvram-format.c b/core/nvram-format.c > index 3d030a30..ed8af561 100644 > --- a/core/nvram-format.c > +++ b/core/nvram-format.c > @@ -216,6 +216,9 @@ const char *nvram_query(const char *key) > const char *part_end, *start; > int key_len = strlen(key); > > + if (!skiboot_part_hdr) > + return NULL; > + > if (!nvram_has_loaded()) { > prlog(PR_WARNING, "NVRAM: Query before is done loading\n"); > prlog(PR_WARNING, "NVRAM: Waiting for load\n"); > -- > 2.16.1 > > _______________________________________________ > Skiboot mailing list > Skiboot@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/skiboot
On Mon, 26 Mar 2018 10:50:31 +1100 Oliver <oohall@gmail.com> wrote: > On Sun, Mar 25, 2018 at 11:48 AM, Nicholas Piggin <npiggin@gmail.com> wrote: > > If nvram_check fails, leaving skiboot_part_hdr == NULL, do not > > have nvram_query proceed using that pointer, just return NULL. > > Shouldn't the checking in nvram_validate() catch that case? It doesn't. > If not we > should probably fix it there. Yeah, good thinking. Looks like this turned out to be a deeper bug. What do you reckon about this? --- core/nvram-format.c | 3 +++ core/nvram.c | 8 +++++--- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/core/nvram-format.c b/core/nvram-format.c index 3d030a30..42c5cbbb 100644 --- a/core/nvram-format.c +++ b/core/nvram-format.c @@ -180,6 +180,7 @@ int nvram_check(void *nvram_image, const uint32_t nvram_size) } prlog(PR_INFO, "NVRAM: Layout appears sane\n"); + assert(skiboot_part_hdr); return 0; failed: return -1; @@ -234,6 +235,8 @@ const char *nvram_query(const char *key) if (!nvram_validate()) return NULL; + assert(skiboot_part_hdr); + part_end = (const char *) skiboot_part_hdr + be16_to_cpu(skiboot_part_hdr->len) * 16 - 1; diff --git a/core/nvram.c b/core/nvram.c index de6cbddf..1c3cdfaf 100644 --- a/core/nvram.c +++ b/core/nvram.c @@ -69,8 +69,10 @@ opal_call(OPAL_WRITE_NVRAM, opal_write_nvram, 3); bool nvram_validate(void) { - if (!nvram_valid) - nvram_valid = !nvram_check(nvram_image, nvram_size); + if (!nvram_valid) { + if (!nvram_check(nvram_image, nvram_size)) + nvram_valid = true; + } return nvram_valid; } @@ -87,7 +89,7 @@ static void nvram_reformat(void) if (platform.nvram_write) platform.nvram_write(0, nvram_image, nvram_size); - nvram_valid = true; + nvram_validate(); } void nvram_reinit(void)
On Mon, Mar 26, 2018 at 11:15 AM, Nicholas Piggin <npiggin@gmail.com> wrote: > On Mon, 26 Mar 2018 10:50:31 +1100 > Oliver <oohall@gmail.com> wrote: > >> On Sun, Mar 25, 2018 at 11:48 AM, Nicholas Piggin <npiggin@gmail.com> wrote: >> > If nvram_check fails, leaving skiboot_part_hdr == NULL, do not >> > have nvram_query proceed using that pointer, just return NULL. >> >> Shouldn't the checking in nvram_validate() catch that case? > > It doesn't. Bleh, we need to sort out the mess of global nvram flag variables at some point. >> If not we >> should probably fix it there. > > Yeah, good thinking. Looks like this turned out to be a deeper bug. > What do you reckon about this? Looks good. Reviewed-by: Oliver O'Halloran <oohall@gmail.com> > > --- > core/nvram-format.c | 3 +++ > core/nvram.c | 8 +++++--- > 2 files changed, 8 insertions(+), 3 deletions(-) > > diff --git a/core/nvram-format.c b/core/nvram-format.c > index 3d030a30..42c5cbbb 100644 > --- a/core/nvram-format.c > +++ b/core/nvram-format.c > @@ -180,6 +180,7 @@ int nvram_check(void *nvram_image, const uint32_t nvram_size) > } > > prlog(PR_INFO, "NVRAM: Layout appears sane\n"); > + assert(skiboot_part_hdr); > return 0; > failed: > return -1; > @@ -234,6 +235,8 @@ const char *nvram_query(const char *key) > if (!nvram_validate()) > return NULL; > > + assert(skiboot_part_hdr); > + > part_end = (const char *) skiboot_part_hdr > + be16_to_cpu(skiboot_part_hdr->len) * 16 - 1; > > diff --git a/core/nvram.c b/core/nvram.c > index de6cbddf..1c3cdfaf 100644 > --- a/core/nvram.c > +++ b/core/nvram.c > @@ -69,8 +69,10 @@ opal_call(OPAL_WRITE_NVRAM, opal_write_nvram, 3); > > bool nvram_validate(void) > { > - if (!nvram_valid) > - nvram_valid = !nvram_check(nvram_image, nvram_size); > + if (!nvram_valid) { > + if (!nvram_check(nvram_image, nvram_size)) > + nvram_valid = true; > + } > > return nvram_valid; > } > @@ -87,7 +89,7 @@ static void nvram_reformat(void) > if (platform.nvram_write) > platform.nvram_write(0, nvram_image, nvram_size); > > - nvram_valid = true; > + nvram_validate(); > } > > void nvram_reinit(void) > -- > 2.16.1 >
diff --git a/core/nvram-format.c b/core/nvram-format.c index 3d030a30..ed8af561 100644 --- a/core/nvram-format.c +++ b/core/nvram-format.c @@ -216,6 +216,9 @@ const char *nvram_query(const char *key) const char *part_end, *start; int key_len = strlen(key); + if (!skiboot_part_hdr) + return NULL; + if (!nvram_has_loaded()) { prlog(PR_WARNING, "NVRAM: Query before is done loading\n"); prlog(PR_WARNING, "NVRAM: Waiting for load\n");
If nvram_check fails, leaving skiboot_part_hdr == NULL, do not have nvram_query proceed using that pointer, just return NULL. mambo can run into this. Signed-off-by: Nicholas Piggin <npiggin@gmail.com> --- core/nvram-format.c | 3 +++ 1 file changed, 3 insertions(+)