efi_loader: image_loader - Coverity Scan errors

[Heinrich Schuchardt]

Hello Takahiro,

this relates to your patch

4540dabdcaca
efi_loader: image_loader: support image authentication

On 4/29/20 4:16 PM, Tom Rini wrote:
> Can you please look in to these, thanks!
>
> ---------- Forwarded message ---------
> From: <scan-admin at coverity.com>
> Date: Tue, Apr 28, 2020 at 4:12 PM
> Subject: New Defects reported by Coverity Scan for Das U-Boot
> To: <tom.rini at gmail.com>
>
>
> Hi,
>
> Please find the latest report on new defect(s) introduced to Das
> U-Boot found with Coverity Scan.
>
> 12 new defect(s) introduced to Das U-Boot found with Coverity Scan.
>
>
> New defect(s) Reported-by: Coverity Scan
> Showing 12 of 12 defect(s)
>
>
> ** CID 300339:    (ARRAY_VS_SINGLETON)
> /lib/efi_loader/efi_image_loader.c: 299 in efi_image_parse()
> /lib/efi_loader/efi_image_loader.c: 294 in efi_image_parse()
> /lib/efi_loader/efi_image_loader.c: 315 in efi_image_parse()
>
>
> ________________________________________________________________________________________________________
> *** CID 300339:    (ARRAY_VS_SINGLETON)
> /lib/efi_loader/efi_image_loader.c: 299 in efi_image_parse()
> 293                     if (nt64->OptionalHeader.NumberOfRvaAndSizes <= ctidx) {
> 294                             efi_image_region_add(regs,
> 295                                                  &opt->CheckSum + 1,

+1 is not adding the size of CheckSum to the address but the size of
IMAGE_OPTIONAL_HEADER64. This cannot be correct.

> 296                                                  efi +
> opt->SizeOfHeaders, 0);
> 297                     } else {
> 298                             /* Skip Certificates Table */
>>>>     CID 300339:    (ARRAY_VS_SINGLETON)
>>>>     Using "&opt->CheckSum" as an array.  This might corrupt or misinterpret adjacent memory locations.
> 299                             efi_image_region_add(regs,
> 300                                                  &opt->CheckSum + 1,

Same here

> 301
> &opt->DataDirectory[ctidx], 0);
> 302                             efi_image_region_add(regs,
> 303
> &opt->DataDirectory[ctidx] + 1,

Same here

> 304                                                  efi +
> opt->SizeOfHeaders, 0);
> /lib/efi_loader/efi_image_loader.c: 294 in efi_image_parse()
> 288                     IMAGE_NT_HEADERS64 *nt64 = (void *)nt;
> 289                     IMAGE_OPTIONAL_HEADER64 *opt = &nt64->OptionalHeader;
> 290
> 291                     /* Skip CheckSum */
> 292                     efi_image_region_add(regs, efi, &opt->CheckSum, 0);
> 293                     if (nt64->OptionalHeader.NumberOfRvaAndSizes <= ctidx) {
>>>>     CID 300339:    (ARRAY_VS_SINGLETON)
>>>>     Using "&opt->CheckSum" as an array.  This might corrupt or misinterpret adjacent memory locations.
> 294                             efi_image_region_add(regs,
> 295                                                  &opt->CheckSum + 1,
> 296                                                  efi +
> opt->SizeOfHeaders, 0);
> 297                     } else {
> 298                             /* Skip Certificates Table */
> 299                             efi_image_region_add(regs,
> /lib/efi_loader/efi_image_loader.c: 315 in efi_image_parse()
> 309                     authoff = opt->DataDirectory[ctidx].VirtualAddress;
> 310                     authsz = opt->DataDirectory[ctidx].Size;
> 311             } else if (nt->OptionalHeader.Magic ==
> IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
> 312                     IMAGE_OPTIONAL_HEADER32 *opt = &nt->OptionalHeader;
> 313
> 314                     efi_image_region_add(regs, efi, &opt->CheckSum, 0);
>>>>     CID 300339:    (ARRAY_VS_SINGLETON)
>>>>     Using "&opt->CheckSum" as an array.  This might corrupt or misinterpret adjacent memory locations.
> 315                     efi_image_region_add(regs, &opt->CheckSum + 1,

Same here

> 316                                          &opt->DataDirectory[ctidx], 0);
> 317                     efi_image_region_add(regs,
> &opt->DataDirectory[ctidx] + 1,
> 318                                          efi + opt->SizeOfHeaders, 0);
> 319
> 320                     bytes_hashed = opt->SizeOfHeaders;
>

Please, check the coding and provide a patch.

Best regards

Heinrich