| Message ID | 20170919043806.12972-1-sjitindarsingh@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | core/flash: Only expect ELF header for BOOTKERNEL partition flash resource | expand |
On 01/01/1970 05:30 AM, wrote: > When loading a flash resource which isn't signed (secure and trusted > boot) and which doesn't have a subpartition, we assume it's the > BOOTKERNEL since previously this was the only such resource. Thus we > also assumed it had an ELF header which we parsed to get the size of the > partition rather than trusting the actual_size field in the FFS header. > A previous commit (9727fe3 DT: Add ibm,firmware-versions node) added the > version resource which isn't signed and also doesn't have a subpartition, > thus we expect it to have an ELF header. It doesn't so we print the > error message "FLASH: Invalid ELF header part VERSION". > > It is a fluke that this works currently since we load the secure boot > header unconditionally and this happen to be the same size as the > version partition. We also don't update the return code on error so > happen to return OPAL_SUCCESS. I was about to send fix for this issue. :-) In my fix I was skipping ELF header checking for RESOURCE_ID_VERSION. But if only BOOTKERNEL has ELF header, then this patch is better. Yeah. We have existing bug in this code and hence ibm,firmware-versions patch worked fine. My bad. I should have caught this earlier :-( > > To make this explicitly correct; only check for an ELF header if we are > loading the BOOTKERNEL resource, otherwise use the partition size from > the FFS header. Also set the return code on error so we don't > erroneously return OPAL_SUCCESS. Add a check that the resource will fit > in the supplied buffer to prevent buffer overrun. > > Fixes: 9727fe3 (DT: Add ibm,firmware-versions node) > > Reported-by: Pridhiviraj Paidipeddi <ppaidipe@in.ibm.com> > Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com> Patch looks good to me and works works fine on my P9 box :-) Reviewed-by: Vasant Hegde <hegdevasant@linux.vnet.ibm.com> -Vasant
On Tue, 2017-09-19 at 14:38 +1000, Suraj Jitindar Singh wrote: > When loading a flash resource which isn't signed (secure and trusted > boot) and which doesn't have a subpartition, we assume it's the > BOOTKERNEL since previously this was the only such resource. Thus we > also assumed it had an ELF header which we parsed to get the size of > the > partition rather than trusting the actual_size field in the FFS > header. > A previous commit (9727fe3 DT: Add ibm,firmware-versions node) added > the > version resource which isn't signed and also doesn't have a > subpartition, > thus we expect it to have an ELF header. It doesn't so we print the > error message "FLASH: Invalid ELF header part VERSION". > > It is a fluke that this works currently since we load the secure boot > header unconditionally and this happen to be the same size as the > version partition. We also don't update the return code on error so > happen to return OPAL_SUCCESS. > > To make this explicitly correct; only check for an ELF header if we > are > loading the BOOTKERNEL resource, otherwise use the partition size > from > the FFS header. Also set the return code on error so we don't > erroneously return OPAL_SUCCESS. Add a check that the resource will > fit > in the supplied buffer to prevent buffer overrun. > > Fixes: 9727fe3 (DT: Add ibm,firmware-versions node) > Stewart: Please replace following line with: Reported-by: Pridhiviraj Paidipeddi <ppaidipe@linux.vnet.ibm.com> Before merging. Or do you want a V2 for that? > Reported-by: Pridhiviraj Paidipeddi <ppaidipe@in.ibm.com> > Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com> > --- > core/flash.c | 32 +++++++++++++++++++++++--------- > 1 file changed, 23 insertions(+), 9 deletions(-) > > diff --git a/core/flash.c b/core/flash.c > index feb7119..6255a65 100644 > --- a/core/flash.c > +++ b/core/flash.c > @@ -692,6 +692,7 @@ static int flash_load_resource(enum resource_id > id, uint32_t subid, > > if (content_size > bufsz) { > prerror("FLASH: content size > buffer > size\n"); > + rc = OPAL_PARAMETER; > goto out_free_ffs; > } > > @@ -725,15 +726,28 @@ static int flash_load_resource(enum resource_id > id, uint32_t subid, > * Back to the old way of doing things, no STB > header. > */ > if (subid == RESOURCE_SUBID_NONE) { > - /* > - * Because actualSize is a lie, we compute > the size > - * of the BOOTKERNEL based on what the ELF > headers > - * say. Otherwise we end up reading more > than we should > - */ > - content_size = sizeof_elf_from_hdr(buf); > - if (!content_size) { > - prerror("FLASH: Invalid ELF header > part %s\n", > - name); > + if (id == RESOURCE_ID_KERNEL) { > + /* > + * Because actualSize is a lie, we > compute the > + * size of the BOOTKERNEL based on > what the ELF > + * headers say. Otherwise we end up > reading more > + * than we should > + */ > + content_size = > sizeof_elf_from_hdr(buf); > + if (!content_size) { > + prerror("FLASH: Invalid ELF > header part" > + " %s\n", name); > + rc = OPAL_RESOURCE; > + goto out_free_ffs; > + } > + } else { > + content_size = ffs_part_size; > + } > + if (content_size > bufsz) { > + prerror("FLASH: %s content size %d > > " > + " buffer size %lu\n", name, > + content_size, bufsz); > + rc = OPAL_PARAMETER; > goto out_free_ffs; > } > prlog(PR_DEBUG, "FLASH: computed %s size > %u\n",
Suraj Jitindar Singh <sjitindarsingh@gmail.com> writes: > Stewart: Please replace following line with: > > Reported-by: Pridhiviraj Paidipeddi <ppaidipe@linux.vnet.ibm.com> > > Before merging. Or do you want a V2 for that? I did it when merging. Merged to master as of eeb4d3226d4eda49ca171d442e49ceccbcaf634e
diff --git a/core/flash.c b/core/flash.c index feb7119..6255a65 100644 --- a/core/flash.c +++ b/core/flash.c @@ -692,6 +692,7 @@ static int flash_load_resource(enum resource_id id, uint32_t subid, if (content_size > bufsz) { prerror("FLASH: content size > buffer size\n"); + rc = OPAL_PARAMETER; goto out_free_ffs; } @@ -725,15 +726,28 @@ static int flash_load_resource(enum resource_id id, uint32_t subid, * Back to the old way of doing things, no STB header. */ if (subid == RESOURCE_SUBID_NONE) { - /* - * Because actualSize is a lie, we compute the size - * of the BOOTKERNEL based on what the ELF headers - * say. Otherwise we end up reading more than we should - */ - content_size = sizeof_elf_from_hdr(buf); - if (!content_size) { - prerror("FLASH: Invalid ELF header part %s\n", - name); + if (id == RESOURCE_ID_KERNEL) { + /* + * Because actualSize is a lie, we compute the + * size of the BOOTKERNEL based on what the ELF + * headers say. Otherwise we end up reading more + * than we should + */ + content_size = sizeof_elf_from_hdr(buf); + if (!content_size) { + prerror("FLASH: Invalid ELF header part" + " %s\n", name); + rc = OPAL_RESOURCE; + goto out_free_ffs; + } + } else { + content_size = ffs_part_size; + } + if (content_size > bufsz) { + prerror("FLASH: %s content size %d > " + " buffer size %lu\n", name, + content_size, bufsz); + rc = OPAL_PARAMETER; goto out_free_ffs; } prlog(PR_DEBUG, "FLASH: computed %s size %u\n",
When loading a flash resource which isn't signed (secure and trusted boot) and which doesn't have a subpartition, we assume it's the BOOTKERNEL since previously this was the only such resource. Thus we also assumed it had an ELF header which we parsed to get the size of the partition rather than trusting the actual_size field in the FFS header. A previous commit (9727fe3 DT: Add ibm,firmware-versions node) added the version resource which isn't signed and also doesn't have a subpartition, thus we expect it to have an ELF header. It doesn't so we print the error message "FLASH: Invalid ELF header part VERSION". It is a fluke that this works currently since we load the secure boot header unconditionally and this happen to be the same size as the version partition. We also don't update the return code on error so happen to return OPAL_SUCCESS. To make this explicitly correct; only check for an ELF header if we are loading the BOOTKERNEL resource, otherwise use the partition size from the FFS header. Also set the return code on error so we don't erroneously return OPAL_SUCCESS. Add a check that the resource will fit in the supplied buffer to prevent buffer overrun. Fixes: 9727fe3 (DT: Add ibm,firmware-versions node) Reported-by: Pridhiviraj Paidipeddi <ppaidipe@in.ibm.com> Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com> --- core/flash.c | 32 +++++++++++++++++++++++--------- 1 file changed, 23 insertions(+), 9 deletions(-)