Re: Fwd: New Defects reported by Coverity Scan for Das U-Boot

Heinrich Schuchardt
2024-01-28T08:51:40Z

On 1/27/24 21:56, Heinrich Schuchardt wrote:
>
>
> Am 27. Januar 2024 16:40:18 MEZ schrieb Tom Rini <trini@konsulko.com>:
>> Hey, I'll just pass this on directly rather than to the list.
>>
>> ---------- Forwarded message ---------
>> From: <scan-admin@coverity.com>
>> Date: Sat, Jan 27, 2024 at 10:36 AM
>> Subject: New Defects reported by Coverity Scan for Das U-Boot
>> To: <tom.rini@gmail.com>
>>
>>
>> Hi,
>>
>> Please find the latest report on new defect(s) introduced to Das
>> U-Boot found with Coverity Scan.
>>
>> 1 new defect(s) introduced to Das U-Boot found with Coverity Scan.
>>
>>
>> New defect(s) Reported-by: Coverity Scan
>> Showing 1 of 1 defect(s)
>>
>>
>> ** CID 479279: (TAINTED_SCALAR)
>>
>>
>> ________________________________________________________________________________________________________
>> *** CID 479279: (TAINTED_SCALAR)
>> /cmd/smbios.c: 180 in do_smbios()
>> 174 smbios_print_type2((struct smbios_type2 *)pos);
>> 175 break;
>> 176 case 127:
>> 177 smbios_print_type127((struct
>> smbios_type127 *)pos);
>> 178 break;
>> 179 default:
>>>>> CID 479279: (TAINTED_SCALAR)
>>>>> Passing tainted expression "pos->length" to "smbios_print_generic", which uses it as a loop boundary.
>> 180 smbios_print_generic(pos);
>> 181 break;
>> 182 }
>> 183 }
>> 184
>> 185 return CMD_RET_SUCCESS;
>> /cmd/smbios.c: 154 in do_smbios()
>> 148 size = entry2->length;
>> 149 max_struct_size = entry2->max_struct_size;
>> 150 } else {
>> 151 log_err("Unknown SMBIOS anchor format\n");
>> 152 return CMD_RET_FAILURE;
>> 153 }
>>>>> CID 479279: (TAINTED_SCALAR)
>>>>> Passing tainted expression "size" to "table_compute_checksum", which uses it as a loop boundary.
>> 154 if (table_compute_checksum(entry, size)) {
>> 155 log_err("Invalid anchor checksum\n");
>> 156 return CMD_RET_FAILURE;
>> 157 }
>> 158 printf("SMBIOS %s present.\n", version);
>> 159
>> /cmd/smbios.c: 174 in do_smbios()
>> 168 (unsigned long long)map_to_sysmem(pos));
>> 169 switch (pos->type) {
>> 170 case 1:
>> 171 smbios_print_type1((struct smbios_type1 *)pos);
>> 172 break;
>> 173 case 2:
>>>>> CID 479279: (TAINTED_SCALAR)
>>>>> Passing tainted expression "((struct smbios_type2 *)pos)->number_contained_objects" to "smbios_print_type2", which uses it as a loop boundary.
>> 174 smbios_print_type2((struct smbios_type2 *)pos);
>> 175 break;
>> 176 case 127:
>> 177 smbios_print_type127((struct
>> smbios_type127 *)pos);
>> 178 break;
>> 179 default:
>> /cmd/smbios.c: 154 in do_smbios()
>> 148 size = entry2->length;
>> 149 max_struct_size = entry2->max_struct_size;
>> 150 } else {
>> 151 log_err("Unknown SMBIOS anchor format\n");
>> 152 return CMD_RET_FAILURE;
>> 153 }
>>>>> CID 479279: (TAINTED_SCALAR)
>>>>> Passing tainted expression "size" to "table_compute_checksum", which uses it as a loop boundary.
>> 154 if (table_compute_checksum(entry, size)) {
>> 155 log_err("Invalid anchor checksum\n");
>> 156 return CMD_RET_FAILURE;
>> 157 }
>> 158 printf("SMBIOS %s present.\n", version);
>> 159
>> /cmd/smbios.c: 180 in do_smbios()
>> 174 smbios_print_type2((struct smbios_type2 *)pos);
>> 175 break;
>> 176 case 127:
>> 177 smbios_print_type127((struct
>> smbios_type127 *)pos);
>> 178 break;
>> 179 default:
>>>>> CID 479279: (TAINTED_SCALAR)
>>>>> Passing tainted expression "pos->length" to "smbios_print_generic", which uses it as a loop boundary.
>> 180 smbios_print_generic(pos);
>> 181 break;
>> 182 }
>> 183 }
>> 184
>> 185 return CMD_RET_SUCCESS;
>> /cmd/smbios.c: 174 in do_smbios()
>> 168 (unsigned long long)map_to_sysmem(pos));
>> 169 switch (pos->type) {
>> 170 case 1:
>> 171 smbios_print_type1((struct smbios_type1 *)pos);
>> 172 break;
>> 173 case 2:
>>>>> CID 479279: (TAINTED_SCALAR)
>>>>> Passing tainted expression "((struct smbios_type2 *)pos)->number_contained_objects" to "smbios_print_type2", which uses it as a loop boundary.
>> 174 smbios_print_type2((struct smbios_type2 *)pos);
>> 175 break;
>> 176 case 127:
>> 177 smbios_print_type127((struct
>> smbios_type127 *)pos);
>> 178 break;
>> 179 default:
>> /cmd/smbios.c: 174 in do_smbios()
>> 168 (unsigned long long)map_to_sysmem(pos));
>> 169 switch (pos->type) {
>> 170 case 1:
>> 171 smbios_print_type1((struct smbios_type1 *)pos);
>> 172 break;
>> 173 case 2:
>>>>> CID 479279: (TAINTED_SCALAR)
>>>>> Passing tainted expression "((struct smbios_type2 *)pos)->number_contained_objects" to "smbios_print_type2", which uses it as a loop boundary.
>> 174 smbios_print_type2((struct smbios_type2 *)pos);
>> 175 break;
>> 176 case 127:
>> 177 smbios_print_type127((struct
>> smbios_type127 *)pos);
>> 178 break;
>> 179 default:
>> /cmd/smbios.c: 180 in do_smbios()
>> 174 smbios_print_type2((struct smbios_type2 *)pos);
>> 175 break;
>> 176 case 127:
>> 177 smbios_print_type127((struct
>> smbios_type127 *)pos);
>> 178 break;
>> 179 default:
>>>>> CID 479279: (TAINTED_SCALAR)
>>>>> Passing tainted expression "pos->length" to "smbios_print_generic", which uses it as a loop boundary.
>> 180 smbios_print_generic(pos);
>> 181 break;
>> 182 }
>> 183 }
>> 184
>> 185 return CMD_RET_SUCCESS;
>> /cmd/smbios.c: 180 in do_smbios()
>> 174 smbios_print_type2((struct smbios_type2 *)pos);
>> 175 break;
>> 176 case 127:
>> 177 smbios_print_type127((struct
>> smbios_type127 *)pos);
>> 178 break;
>> 179 default:
>>>>> CID 479279: (TAINTED_SCALAR)
>>>>> Passing tainted expression "pos->length" to "smbios_print_generic", which uses it as a loop boundary.
>> 180 smbios_print_generic(pos);
>> 181 break;
>> 182 }
>> 183 }
>> 184
>> 185 return CMD_RET_SUCCESS;
>> /cmd/smbios.c: 174 in do_smbios()
>> 168 (unsigned long long)map_to_sysmem(pos));
>> 169 switch (pos->type) {
>> 170 case 1:
>> 171 smbios_print_type1((struct smbios_type1 *)pos);
>> 172 break;
>> 173 case 2:
>>>>> CID 479279: (TAINTED_SCALAR)
>>>>> Passing tainted expression "((struct smbios_type2 *)pos)->number_contained_objects" to "smbios_print_type2", which uses it as a loop boundary.
>> 174 smbios_print_type2((struct smbios_type2 *)pos);
>> 175 break;
>> 176 case 127:
>> 177 smbios_print_type127((struct
>> smbios_type127 *)pos);
>> 178 break;
>> 179 default:
>> /cmd/smbios.c: 180 in do_smbios()
>> 174 smbios_print_type2((struct smbios_type2 *)pos);
>> 175 break;
>> 176 case 127:
>> 177 smbios_print_type127((struct
>> smbios_type127 *)pos);
>> 178 break;
>> 179 default:
>>>>> CID 479279: (TAINTED_SCALAR)
>>>>> Passing tainted expression "pos->length" to "smbios_print_generic", which uses it as a loop boundary.
>> 180 smbios_print_generic(pos);
>> 181 break;
>> 182 }
>> 183 }
>> 184
>> 185 return CMD_RET_SUCCESS;
>> /cmd/smbios.c: 174 in do_smbios()
>> 168 (unsigned long long)map_to_sysmem(pos));
>> 169 switch (pos->type) {
>> 170 case 1:
>> 171 smbios_print_type1((struct smbios_type1 *)pos);
>> 172 break;
>> 173 case 2:
>>>>> CID 479279: (TAINTED_SCALAR)
>>>>> Passing tainted expression "((struct smbios_type2 *)pos)->number_contained_objects" to "smbios_print_type2", which uses it as a loop boundary.
>> 174 smbios_print_type2((struct smbios_type2 *)pos);
>> 175 break;
>> 176 case 127:
>> 177 smbios_print_type127((struct
>> smbios_type127 *)pos);
>> 178 break;
>> 179 default:
>>
>
> The values may come from QEMU, so may be "tainted". We could check the length of the individual structures against the total size of the SMBIOS table.
>

In Coverity I marked this as false positive with the following comment:

"The only case in which the data is tainted is when copying the smbios
table from a prior firmware state when running as EFI app or from QEMU.
Sanity checks should not be in the smbios command but where we import
the table."

Best regards

Heinrich