[Skiboot] [SKIBOOT] [PATCH] core/flash: Only expect ELF header for BOOTKERNEL partition flash resource

[Suraj Jitindar Singh]

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 at linux.vnet.ibm.com>

Before merging. Or do you want a V2 for that?

> Reported-by: Pridhiviraj Paidipeddi <ppaidipe at in.ibm.com>
> Signed-off-by: Suraj Jitindar Singh <sjitindarsingh at 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",