Fwd: New Defects reported by Coverity Scan for Das U-Boot
Here's the latest report.
-- Forwarded message -
From:
Date: Mon, Apr 22, 2024 at 3:23 PM
Subject: New Defects reported by Coverity Scan for Das U-Boot
To:
Hi,
Please find the latest report on new defect(s) introduced to Das
U-Boot found with Coverity Scan.
2 new defect(s) introduced to Das U-Boot found with Coverity Scan.
7 defect(s), reported by Coverity Scan earlier, were marked fixed in
the recent build analyzed by Coverity Scan.
New defect(s) Reported-by: Coverity Scan
Showing 2 of 2 defect(s)
** CID 492766: Control flow issues (DEADCODE)
/lib/efi_loader/efi_var_mem.c: 236 in efi_var_mem_init()
*** CID 492766: Control flow issues (DEADCODE)
/lib/efi_loader/efi_var_mem.c: 236 in efi_var_mem_init()
230 memset(efi_var_buf, 0, EFI_VAR_BUF_SIZE);
231 efi_var_buf->magic = EFI_VAR_FILE_MAGIC;
232 efi_var_buf->length = (uintptr_t)efi_var_buf->var -
233 (uintptr_t)efi_var_buf;
234
235 if (ret != EFI_SUCCESS)
>>> CID 492766: Control flow issues (DEADCODE)
>>> Execution cannot reach this statement: "return ret;".
236 return ret;
237 ret =
efi_create_event(EVT_SIGNAL_VIRTUAL_ADDRESS_CHANGE, TPL_CALLBACK,
238
efi_var_mem_notify_virtual_address_map, NULL,
239NULL, );
240 if (ret != EFI_SUCCESS)
241 return ret;
** CID 492765: Uninitialized variables (UNINIT)
*** CID 492765: Uninitialized variables (UNINIT)
/net/bootp.c: 888 in dhcp_process_options()
882 net_root_path[size] = 0;
883 break;
884 case 28:/* Ignore Broadcast Address Option */
885 break;
886 case 40:/* NIS Domain name */
887 if (net_nis_domain[0] == 0) {
>>> CID 492765: Uninitialized variables (UNINIT)
>>> Using uninitialized value "size" when calling "truncate_sz".
888 size = truncate_sz("NIS Domain Name",
889 sizeof(net_nis_domain), size);
890 memcpy(_nis_domain, popt + 2, size);
891 net_nis_domain[size] = 0;
892 }
893 break;
--
Tom
signature.asc
Description: PGP signature
Re: Fwd: New Defects reported by Coverity Scan for Das U-Boot
On 1/30/24 00:55, Tom Rini wrote:
Here's the latest report.
-- Forwarded message -
From:
Date: Mon, Jan 29, 2024 at 6:51 PM
Subject: New Defects reported by Coverity Scan for Das U-Boot
To:
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.
1 defect(s), reported by Coverity Scan earlier, were marked fixed in
the recent build analyzed by Coverity Scan.
New defect(s) Reported-by: Coverity Scan
Showing 1 of 1 defect(s)
** CID 480240: Insecure data handling (TAINTED_SCALAR)
/cmd/efidebug.c: 192 in do_efi_capsule_esrt()
*** CID 480240: Insecure data handling (TAINTED_SCALAR)
/cmd/efidebug.c: 192 in do_efi_capsule_esrt()
186
187 printf("\n");
188 printf("ESRT: fw_resource_count=%d\n", esrt->fw_resource_count);
189 printf("ESRT: fw_resource_count_max=%d\n",
esrt->fw_resource_count_max);
190 printf("ESRT: fw_resource_version=%lld\n",
esrt->fw_resource_version);
191
CID 480240: Insecure data handling (TAINTED_SCALAR)
Using tainted variable "esrt->fw_resource_count" as a loop boundary.
192 for (int idx = 0; idx < esrt->fw_resource_count; idx++) {
193 printf("[entry
%d]==\n", idx);
194 printf("ESRT: fw_class=%pUL\n",
>entries[idx].fw_class);
195 printf("ESRT: fw_type=%s\n",
EFI_FW_TYPE_STR_GET(esrt->entries[idx].fw_type));
196 printf("ESRT: fw_version=%d\n",
esrt->entries[idx].fw_version);
197 printf("ESRT: lowest_supported_fw_version=%d\n",
- End forwarded message -
Coverity sees any conversion from void * as a hint to tainted data. The
ESRT might be manipulated by some EFI app but we want to display it. So
I marked this Coverity issue as intentional.
Best regards
Heinrich
Fwd: New Defects reported by Coverity Scan for Das U-Boot
Here's the latest report.
-- Forwarded message -
From:
Date: Mon, Jan 29, 2024 at 6:51 PM
Subject: New Defects reported by Coverity Scan for Das U-Boot
To:
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.
1 defect(s), reported by Coverity Scan earlier, were marked fixed in
the recent build analyzed by Coverity Scan.
New defect(s) Reported-by: Coverity Scan
Showing 1 of 1 defect(s)
** CID 480240: Insecure data handling (TAINTED_SCALAR)
/cmd/efidebug.c: 192 in do_efi_capsule_esrt()
*** CID 480240: Insecure data handling (TAINTED_SCALAR)
/cmd/efidebug.c: 192 in do_efi_capsule_esrt()
186
187 printf("\n");
188 printf("ESRT: fw_resource_count=%d\n", esrt->fw_resource_count);
189 printf("ESRT: fw_resource_count_max=%d\n",
esrt->fw_resource_count_max);
190 printf("ESRT: fw_resource_version=%lld\n",
esrt->fw_resource_version);
191
>>> CID 480240: Insecure data handling (TAINTED_SCALAR)
>>> Using tainted variable "esrt->fw_resource_count" as a loop boundary.
192 for (int idx = 0; idx < esrt->fw_resource_count; idx++) {
193 printf("[entry
%d]==\n", idx);
194 printf("ESRT: fw_class=%pUL\n",
>entries[idx].fw_class);
195 printf("ESRT: fw_type=%s\n",
EFI_FW_TYPE_STR_GET(esrt->entries[idx].fw_type));
196 printf("ESRT: fw_version=%d\n",
esrt->entries[idx].fw_version);
197 printf("ESRT: lowest_supported_fw_version=%d\n",
- End forwarded message -
--
Tom
signature.asc
Description: PGP signature
Re: Fwd: New Defects reported by Coverity Scan for Das U-Boot
On 1/27/24 21:56, Heinrich Schuchardt wrote:
Am 27. Januar 2024 16:40:18 MEZ schrieb Tom Rini :
Hey, I'll just pass this on directly rather than to the list.
-- Forwarded message -
From:
Date: Sat, Jan 27, 2024 at 10:36 AM
Subject: New Defects reported by Coverity Scan for Das U-Boot
To:
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
Re: Fwd: New Defects reported by Coverity Scan for Das U-Boot
Am 27. Januar 2024 16:40:18 MEZ schrieb Tom Rini :
>Hey, I'll just pass this on directly rather than to the list.
>
>-- Forwarded message -
>From:
>Date: Sat, Jan 27, 2024 at 10:36 AM
>Subject: New Defects reported by Coverity Scan for Das U-Boot
>To:
>
>
>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
Re: Fwd: New Defects reported by Coverity Scan for Das U-Boot
Hi Tom, sorry about that. Please find attached a patch. Can you please review? Thanks, Hugo
Fwd: New Defects reported by Coverity Scan for Das U-Boot
I've now updated to the latest Coverity scan tool and that eliminated
some previous defects and found two new ones:
-- Forwarded message -
From:
Date: Mon, Jan 22, 2024 at 6:42 PM
Subject: New Defects reported by Coverity Scan for Das U-Boot
To:
Hi,
Please find the latest report on new defect(s) introduced to Das
U-Boot found with Coverity Scan.
2 new defect(s) introduced to Das U-Boot found with Coverity Scan.
8 defect(s), reported by Coverity Scan earlier, were marked fixed in
the recent build analyzed by Coverity Scan.
New defect(s) Reported-by: Coverity Scan
Showing 2 of 2 defect(s)
** CID 478862: Memory - corruptions (OVERRUN)
*** CID 478862: Memory - corruptions (OVERRUN)
/lib/initcall.c: 82 in initcall_run_list()
76 if (ret) {
77 if (CONFIG_IS_ENABLED(EVENT)) {
78 char buf[60];
79
80 /* don't worry about buf size as we are dying here */
81 if (type) {
>>> CID 478862: Memory - corruptions (OVERRUN)
>>> Overrunning callee's array of size 15 by passing argument "type" (which
>>> evaluates to 255) in call to "event_type_name".
82 sprintf(buf, "event %d/%s", type,
83 event_type_name(type));
84 } else {
85 sprintf(buf, "call %p", func);
86 }
87
** CID 478861: Memory - corruptions (OVERRUN)
*** CID 478861: Memory - corruptions (OVERRUN)
/cmd/nvedit.c: 356 in print_static_flags()
350 static int print_static_flags(const char *var_name, const char *flags,
351 void *priv)
352 {
353 enum env_flags_vartype type = env_flags_parse_vartype(flags);
354 enum env_flags_varaccess access =
env_flags_parse_varaccess(flags);
355
>>> CID 478861: Memory - corruptions (OVERRUN)
>>> Overrunning callee's array of size 4 by passing argument "access"
>>> (which evaluates to 4) in call to "env_flags_get_varaccess_name".
356 printf("\t%-20s %-20s %-20s\n", var_name,
357 env_flags_get_vartype_name(type),
358 env_flags_get_varaccess_name(access));
359
360 return 0;
361 }
--
Tom
signature.asc
Description: PGP signature
Fwd: New Defects reported by Coverity Scan for Das U-Boot
Hey all,
Here's the latest Coverity scan report.
-- Forwarded message -
From:
Date: Mon, Jan 22, 2024 at 6:26 PM
Subject: New Defects reported by Coverity Scan for Das U-Boot
To:
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.
7 defect(s), reported by Coverity Scan earlier, were marked fixed in
the recent build analyzed by Coverity Scan.
New defect(s) Reported-by: Coverity Scan
Showing 1 of 1 defect(s)
** CID 478860: Code maintainability issues (UNUSED_VALUE)
/tools/image-host.c: 359 in fit_image_read_key_iv_data()
*** CID 478860: Code maintainability issues (UNUSED_VALUE)
/tools/image-host.c: 359 in fit_image_read_key_iv_data()
353 if (ret >= sizeof(filename)) {
354 printf("Can't format the key or IV filename
when setting up the cipher: insufficient buffer space\n");
355 ret = -1;
356 }
357 if (ret < 0) {
358 printf("Can't format the key or IV filename
when setting up the cipher: snprintf error\n");
>>> CID 478860: Code maintainability issues (UNUSED_VALUE)
>>> Assigning value "-1" to "ret" here, but that stored value is
>>> overwritten before it can be used.
359 ret = -1;
360 }
361
362 ret = fit_image_read_data(filename, key_iv_data, expected_size);
363
364 return ret;
- End forwarded message -
--
Tom
signature.asc
Description: PGP signature
Fwd: New Defects reported by Coverity Scan for Das U-Boot
*** CID 478333: Error handling issues (CHECKED_RETURN) /lib/efi_loader/efi_firmware.c: 413 in efi_firmware_set_fmp_state_var() 407 /* 408 * GetVariable may fail, EFI_NOT_FOUND is returned if FmpState 409 * variable has not been set yet. 410 * Ignore the error here since the correct FmpState variable 411 * is set later. 412 */ CID 478333: Error handling issues (CHECKED_RETURN) Calling "efi_get_variable_int" without checking return value (as is done elsewhere 29 out of 33 times). 413 efi_get_variable_int(varname, image_type_id, NULL, , var_state, 414 NULL); 415 416 /* 417 * Only the fw_version is set here. 418 * lowest_supported_version in FmpState variable is ignored since There are a lot of different return values that may occur when calling efi_get_variable_int, e.g. * EFI_BUFFER_TOO_SMALL * EFI_DEVICE_ERROR Should we overwrite the variable in these cases with NUL values except for var_state[update_bank].fw_version? Best regards Heinrich
Fwd: New Defects reported by Coverity Scan for Das U-Boot
Here's the current set of new issues since I last ran Coverity. -- Forwarded message - From: Date: Thu, Jan 18, 2024 at 9:20 AM Subject: New Defects reported by Coverity Scan for Das U-Boot To: Hi, Please find the latest report on new defect(s) introduced to Das U-Boot found with Coverity Scan. 2 new defect(s) introduced to Das U-Boot found with Coverity Scan. 16 defect(s), reported by Coverity Scan earlier, were marked fixed in the recent build analyzed by Coverity Scan. New defect(s) Reported-by: Coverity Scan Showing 2 of 2 defect(s) ** CID 478334: Memory - corruptions (OVERRUN) *** CID 478334: Memory - corruptions (OVERRUN) /cmd/eficonfig.c: 534 in eficonfig_create_device_path() 528 p += fp_size; 529 *((struct efi_device_path *)p) = END; 530 531 dp = efi_dp_shorten(dp_volume); 532 if (!dp) 533 dp = dp_volume; >>> CID 478334: Memory - corruptions (OVERRUN) >>> Overrunning struct type efi_device_path of 4 bytes by passing it to a >>> function which accesses it at byte offset 5 using argument "fp->dp.length" >>> (which evaluates to 6). 534 dp = efi_dp_concat(dp, >dp, false); 535 free(buf); 536 537 return dp; 538 } 539 ** CID 478333: Error handling issues (CHECKED_RETURN) /lib/efi_loader/efi_firmware.c: 413 in efi_firmware_set_fmp_state_var() *** CID 478333: Error handling issues (CHECKED_RETURN) /lib/efi_loader/efi_firmware.c: 413 in efi_firmware_set_fmp_state_var() 407 /* 408 * GetVariable may fail, EFI_NOT_FOUND is returned if FmpState 409 * variable has not been set yet. 410 * Ignore the error here since the correct FmpState variable 411 * is set later. 412 */ >>> CID 478333: Error handling issues (CHECKED_RETURN) >>> Calling "efi_get_variable_int" without checking return value (as is >>> done elsewhere 29 out of 33 times). 413 efi_get_variable_int(varname, image_type_id, NULL, , var_state, 414 NULL); 415 416 /* 417 * Only the fw_version is set here. 418 * lowest_supported_version in FmpState variable is ignored since -- Tom signature.asc Description: PGP signature
Re: Fwd: New Defects reported by Coverity Scan for Das U-Boot
On Tue, Jan 09, 2024 at 12:26:13AM -0500, Sean Anderson wrote:
> Comments on NAND stuff only.
>
> On 1/8/24 12:45, Tom Rini wrote:
> >
> > *** CID 477216:(BAD_SHIFT)
> > /drivers/mtd/nand/raw/nand_base.c: 3921 in nand_flash_detect_onfi()
> > 3915
> > 3916/*
> > 3917 * pages_per_block and blocks_per_lun may not be a
> > power-of-2 size
> > 3918 * (don't ask me who thought of this...). MTD assumes that
> > these
> > 3919 * dimensions will be power-of-2, so just truncate the
> > remaining area.
> > 3920 */
> > > > > CID 477216:(BAD_SHIFT)
> > > > > In expression "1 <<
> > > > > generic_fls((__u32)(__le32)p->pages_per_block) - 1", shifting by a
> > > > > negative amount has undefined behavior. The shift amount,
> > > > > "generic_fls((__u32)(__le32)p->pages_per_block) - 1", is -1.
> > 3921mtd->erasesize = 1 <<
> > (fls(le32_to_cpu(p->pages_per_block)) - 1);
> > 3922mtd->erasesize *= mtd->writesize;
> > 3923
> > 3924mtd->oobsize = le16_to_cpu(p->spare_bytes_per_page);
> > 3925
> > 3926/* See erasesize comment */
> > /drivers/mtd/nand/raw/nand_base.c: 3927 in nand_flash_detect_onfi()
> > 3921mtd->erasesize = 1 <<
> > (fls(le32_to_cpu(p->pages_per_block)) - 1);
> > 3922mtd->erasesize *= mtd->writesize;
> > 3923
> > 3924mtd->oobsize = le16_to_cpu(p->spare_bytes_per_page);
> > 3925
> > 3926/* See erasesize comment */
> > > > > CID 477216:(BAD_SHIFT)
> > > > > In expression "1 <<
> > > > > generic_fls((__u32)(__le32)p->blocks_per_lun) - 1", shifting by a
> > > > > negative amount has undefined behavior. The shift amount,
> > > > > "generic_fls((__u32)(__le32)p->blocks_per_lun) - 1", is -1.
> > 3927chip->chipsize = 1 << (fls(le32_to_cpu(p->blocks_per_lun))
> > - 1);
> > 3928chip->chipsize *= (uint64_t)mtd->erasesize * p->lun_count;
> > 3929chip->bits_per_cell = p->bits_per_cell;
> > 3930
> > 3931if (onfi_feature(chip) & ONFI_FEATURE_16_BIT_BUS)
> > 3932chip->options |= NAND_BUSWIDTH_16;
>
> Yeah, this looks like a bug.
>
> > ** CID 477215: Control flow issues (MISSING_BREAK)
> > /drivers/mtd/nand/raw/nand_base.c: 4978 in nand_scan_tail()
> >
> >
> >
> > *** CID 477215: Control flow issues (MISSING_BREAK)
> > /drivers/mtd/nand/raw/nand_base.c: 4978 in nand_scan_tail()
> > 4972pr_warn("No ECC functions supplied;
> > hardware ECC not possible\n");
> > 4973BUG();
> > 4974}
> > 4975if (!ecc->read_page)
> > 4976ecc->read_page =
> > nand_read_page_hwecc_oob_first;
> > 4977
> > > > > CID 477215: Control flow issues (MISSING_BREAK)
> > > > > The case for value "NAND_ECC_HW" is not terminated by a "break"
> > > > > statement.
> > 4978case NAND_ECC_HW:
> > 4979/* Use standard hwecc read page function? */
> > 4980if (!ecc->read_page)
> > 4981ecc->read_page = nand_read_page_hwecc;
> > 4982if (!ecc->write_page)
> > 4983ecc->write_page = nand_write_page_hwecc;
>
> I think we just need a fallthrough comment here.
>
> > ** CID 477214: Integer handling issues (BAD_SHIFT)
> > /drivers/mtd/nand/raw/nand_base.c: 4397 in nand_detect()
> >
> >
> >
> > *** CID 477214: Integer handling issues (BAD_SHIFT)
> > /drivers/mtd/nand/raw/nand_base.c: 4397 in nand_detect()
> > 4391
> > 4392nand_decode_bbm_options(mtd, chip);
> > 4393
> > 4394/* Calculate the address shift from the page size */
> > 4395chip->page_shift = ffs(mtd->writesize) - 1;
> > 4396/* Convert chipsize to number of pages per chip -1 */
> > > > > CID 477214: Integer handling issues (BAD_SHIFT)
> > > > > In expression "chip->chipsize >> chip->page_shift", shifting by
> > > > > a negative amount has undefined behavior. The shift amount,
> > > > > "chip->page_shift", is -1.
> > 4397chip->pagemask = (chip->chipsize >> chip->page_shift) - 1;
> > 4398
> > 4399chip->bbt_erase_shift = chip->phys_erase_shift =
> > 4400ffs(mtd->erasesize) - 1;
> > 4401if (chip->chipsize & 0x)
> > 4402chip->chip_shift = ffs((unsigned)chip->chipsize) -
> > 1;
>
> Buggy, but only when writesize is 0 (which is a bigger bug in the nand chip).
>
> > ** CID 477213: Security best practices violations (DC.WEAK_CRYPTO)
>
Re: Fwd: New Defects reported by Coverity Scan for Das U-Boot
Comments on NAND stuff only.
On 1/8/24 12:45, Tom Rini wrote:
*** CID 477216:(BAD_SHIFT)
/drivers/mtd/nand/raw/nand_base.c: 3921 in nand_flash_detect_onfi()
3915
3916/*
3917 * pages_per_block and blocks_per_lun may not be a
power-of-2 size
3918 * (don't ask me who thought of this...). MTD assumes that these
3919 * dimensions will be power-of-2, so just truncate the
remaining area.
3920 */
CID 477216:(BAD_SHIFT)
In expression "1 << generic_fls((__u32)(__le32)p->pages_per_block) - 1", shifting by a
negative amount has undefined behavior. The shift amount,
"generic_fls((__u32)(__le32)p->pages_per_block) - 1", is -1.
3921mtd->erasesize = 1 <<
(fls(le32_to_cpu(p->pages_per_block)) - 1);
3922mtd->erasesize *= mtd->writesize;
3923
3924mtd->oobsize = le16_to_cpu(p->spare_bytes_per_page);
3925
3926/* See erasesize comment */
/drivers/mtd/nand/raw/nand_base.c: 3927 in nand_flash_detect_onfi()
3921mtd->erasesize = 1 <<
(fls(le32_to_cpu(p->pages_per_block)) - 1);
3922mtd->erasesize *= mtd->writesize;
3923
3924mtd->oobsize = le16_to_cpu(p->spare_bytes_per_page);
3925
3926/* See erasesize comment */
CID 477216:(BAD_SHIFT)
In expression "1 << generic_fls((__u32)(__le32)p->blocks_per_lun) - 1", shifting by a
negative amount has undefined behavior. The shift amount,
"generic_fls((__u32)(__le32)p->blocks_per_lun) - 1", is -1.
3927chip->chipsize = 1 << (fls(le32_to_cpu(p->blocks_per_lun)) - 1);
3928chip->chipsize *= (uint64_t)mtd->erasesize * p->lun_count;
3929chip->bits_per_cell = p->bits_per_cell;
3930
3931if (onfi_feature(chip) & ONFI_FEATURE_16_BIT_BUS)
3932chip->options |= NAND_BUSWIDTH_16;
Yeah, this looks like a bug.
** CID 477215: Control flow issues (MISSING_BREAK)
/drivers/mtd/nand/raw/nand_base.c: 4978 in nand_scan_tail()
*** CID 477215: Control flow issues (MISSING_BREAK)
/drivers/mtd/nand/raw/nand_base.c: 4978 in nand_scan_tail()
4972pr_warn("No ECC functions supplied;
hardware ECC not possible\n");
4973BUG();
4974}
4975if (!ecc->read_page)
4976ecc->read_page = nand_read_page_hwecc_oob_first;
4977
CID 477215: Control flow issues (MISSING_BREAK)
The case for value "NAND_ECC_HW" is not terminated by a "break" statement.
4978case NAND_ECC_HW:
4979/* Use standard hwecc read page function? */
4980if (!ecc->read_page)
4981ecc->read_page = nand_read_page_hwecc;
4982if (!ecc->write_page)
4983ecc->write_page = nand_write_page_hwecc;
I think we just need a fallthrough comment here.
** CID 477214: Integer handling issues (BAD_SHIFT)
/drivers/mtd/nand/raw/nand_base.c: 4397 in nand_detect()
*** CID 477214: Integer handling issues (BAD_SHIFT)
/drivers/mtd/nand/raw/nand_base.c: 4397 in nand_detect()
4391
4392nand_decode_bbm_options(mtd, chip);
4393
4394/* Calculate the address shift from the page size */
4395chip->page_shift = ffs(mtd->writesize) - 1;
4396/* Convert chipsize to number of pages per chip -1 */
CID 477214: Integer handling issues (BAD_SHIFT)
In expression "chip->chipsize >> chip->page_shift", shifting by a negative amount has
undefined behavior. The shift amount, "chip->page_shift", is -1.
4397chip->pagemask = (chip->chipsize >> chip->page_shift) - 1;
4398
4399chip->bbt_erase_shift = chip->phys_erase_shift =
4400ffs(mtd->erasesize) - 1;
4401if (chip->chipsize & 0x)
4402chip->chip_shift = ffs((unsigned)chip->chipsize) - 1;
Buggy, but only when writesize is 0 (which is a bigger bug in the nand chip).
** CID 477213: Security best practices violations (DC.WEAK_CRYPTO)
/test/dm/nand.c: 67 in dm_test_nand()
*** CID 477213: Security best practices violations (DC.WEAK_CRYPTO)
/test/dm/nand.c: 67 in dm_test_nand()
61 ops.ooblen = mtd->oobsize;
62 ut_assertok(mtd_read_oob(mtd, mtd->erasesize, ));
63 ut_asserteq(0, oob[mtd_to_nand(mtd)->badblockpos]);
64
65 /* Generate some data and write it */
66 for (i = 0; i < size / sizeof(int); i++)
CID 477213: Security best practices
Fwd: New Defects reported by Coverity Scan for Das U-Boot
Hey all,
Now that I've merged next I've re-run Coverity to get a start on issues
that've been added since last run. The report isn't complete because of
the number of issues, sadly, but if someone is interested in a specific
area contact me off-list and I can provide access to the dashboard.
For the hush related issues, this would be a good chance to work with
upstream and then backport the changes I suspect.
-- Forwarded message -
From:
Date: Mon, Jan 8, 2024 at 12:24 PM
Subject: New Defects reported by Coverity Scan for Das U-Boot
To:
Hi,
Please find the latest report on new defect(s) introduced to Das
U-Boot found with Coverity Scan.
41 new defect(s) introduced to Das U-Boot found with Coverity Scan.
4 defect(s), reported by Coverity Scan earlier, were marked fixed in
the recent build analyzed by Coverity Scan.
New defect(s) Reported-by: Coverity Scan
Showing 20 of 41 defect(s)
** CID 477217: Memory - illegal accesses (NEGATIVE_RETURNS)
/common/cli_hush_upstream.c: 5518 in parse_dollar()
*** CID 477217: Memory - illegal accesses (NEGATIVE_RETURNS)
/common/cli_hush_upstream.c: 5518 in parse_dollar()
5512break;
5513if (--cnt == 0)
5514goto bad_dollar_syntax;
5515if (len_single_ch != '#' &&
strchr(VAR_SUBST_OPS, ch))
5516/* ${NN...} is valid */
5517goto eat_until_closing;
>>> CID 477217: Memory - illegal accesses (NEGATIVE_RETURNS)
>>> Using variable "ch" as an index to array "_ctype".
5518if (!isdigit(ch))
5519goto bad_dollar_syntax;
5520}
5521} else
5522while (1) {
5523unsigned pos;
** CID 477216:(BAD_SHIFT)
/drivers/mtd/nand/raw/nand_base.c: 3921 in nand_flash_detect_onfi()
/drivers/mtd/nand/raw/nand_base.c: 3927 in nand_flash_detect_onfi()
*** CID 477216:(BAD_SHIFT)
/drivers/mtd/nand/raw/nand_base.c: 3921 in nand_flash_detect_onfi()
3915
3916/*
3917 * pages_per_block and blocks_per_lun may not be a
power-of-2 size
3918 * (don't ask me who thought of this...). MTD assumes that these
3919 * dimensions will be power-of-2, so just truncate the
remaining area.
3920 */
>>> CID 477216:(BAD_SHIFT)
>>> In expression "1 << generic_fls((__u32)(__le32)p->pages_per_block) -
>>> 1", shifting by a negative amount has undefined behavior. The shift
>>> amount, "generic_fls((__u32)(__le32)p->pages_per_block) - 1", is -1.
3921mtd->erasesize = 1 <<
(fls(le32_to_cpu(p->pages_per_block)) - 1);
3922mtd->erasesize *= mtd->writesize;
3923
3924mtd->oobsize = le16_to_cpu(p->spare_bytes_per_page);
3925
3926/* See erasesize comment */
/drivers/mtd/nand/raw/nand_base.c: 3927 in nand_flash_detect_onfi()
3921mtd->erasesize = 1 <<
(fls(le32_to_cpu(p->pages_per_block)) - 1);
3922mtd->erasesize *= mtd->writesize;
3923
3924mtd->oobsize = le16_to_cpu(p->spare_bytes_per_page);
3925
3926/* See erasesize comment */
>>> CID 477216:(BAD_SHIFT)
>>> In expression "1 << generic_fls((__u32)(__le32)p->blocks_per_lun) - 1",
>>> shifting by a negative amount has undefined behavior. The shift amount,
>>> "generic_fls((__u32)(__le32)p->blocks_per_lun) - 1", is -1.
3927chip->chipsize = 1 << (fls(le32_to_cpu(p->blocks_per_lun)) - 1);
3928chip->chipsize *= (uint64_t)mtd->erasesize * p->lun_count;
3929chip->bits_per_cell = p->bits_per_cell;
3930
3931if (onfi_feature(chip) & ONFI_FEATURE_16_BIT_BUS)
3932chip->options |= NAND_BUSWIDTH_16;
** CID 477215: Control flow issues (MISSING_BREAK)
/drivers/mtd/nand/raw/nand_base.c: 4978 in nand_scan_tail()
*** CID 477215: Control flow issues (MISSING_BREAK)
/drivers/mtd/nand/raw/nand_base.c: 4978 in nand_scan_tail()
4972pr_warn("No ECC functions supplied;
hardware ECC not possible\n");
4973BUG();
4974}
4975if (!ecc->read_page)
4976ecc->read_page = nand_read_page_hwecc_oob_first;
4977
>>> CID 477215: Control flow issues (MISSING_BREAK)
>>> The case for value "NAND_ECC_HW" is not terminated by a "break"
>>> statement.
4978case NAND_ECC_HW:
Re: [tom.r...@gmail.com: Fwd: New Defects reported by Coverity Scan for Das U-Boot]
On Thu, Dec 21, 2023 at 02:48:31PM +, Shantur Rathore wrote:
> Hi Tom,
>
> On Mon, Dec 18, 2023 at 4:26 PM Tom Rini wrote:
> >
> > Here's the latest report.
> >
> > -- Forwarded message -
> > From:
> > Date: Mon, Dec 18, 2023 at 8:42 AM
> > Subject: New Defects reported by Coverity Scan for Das U-Boot
> > To:
> >
> >
> > 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 470930: Uninitialized variables (UNINIT)
> > /boot/bootmeth_efi.c: 465 in distro_efi_boot()
> >
> >
> >
> > *** CID 470930: Uninitialized variables (UNINIT)
> > /boot/bootmeth_efi.c: 465 in distro_efi_boot()
> > 459 */
> > 460 if (bflow->flags & BOOTFLOWF_USE_BUILTIN_FDT) {
> > 461 log_debug("Booting with built-in fdt\n");
> > 462 snprintf(cmd, sizeof(cmd), "bootefi %lx", kernel);
> > 463 } else {
> > 464 log_debug("Booting with external fdt\n");
> > >>> CID 470930: Uninitialized variables (UNINIT)
> > >>> Using uninitialized value "fdt" when calling "snprintf". [Note: The
> > >>> source code implementation of the function has been overridden by a
> > >>> builtin model.]
> > 465 snprintf(cmd, sizeof(cmd), "bootefi %lx %lx",
> > kernel, fdt);
> > 466 }
> > 467
> > 468 if (run_command(cmd, 0))
> > 469 return log_msg_ret("run", -EINVAL);
> > 470
> >
>
> The code in question is
>
> if (!bootmeth_uses_network(bflow)) {
> // snip
> if (bflow->flags & ~BOOTFLOWF_USE_BUILTIN_FDT)
> fdt = bflow->fdt_addr;
> } else {
> // snip
> fdt = env_get_hex("fdt_addr_r", 0);
> }
> //snip
> if (bflow->flags & BOOTFLOWF_USE_BUILTIN_FDT) {
> log_debug("Booting with built-in fdt\n");
> snprintf(cmd, sizeof(cmd), "bootefi %lx", kernel);
> } else {
> log_debug("Booting with external fdt\n");
> snprintf(cmd, sizeof(cmd), "bootefi %lx %lx", kernel, fdt);
> }
>
> I am unsure in which case is fdt uninitialised.
> Unless the tool is being thrown off by different version of
> bflow->flags & BOOTFLOWF_USE_BUILTIN_FDT
> check, I don't see the problem.
>
> Please let me know if you see it.
Ah, right, thanks for re-checking.
--
Tom
signature.asc
Description: PGP signature
Re: [tom.r...@gmail.com: Fwd: New Defects reported by Coverity Scan for Das U-Boot]
Hi Tom,
On Mon, Dec 18, 2023 at 4:26 PM Tom Rini wrote:
>
> Here's the latest report.
>
> -- Forwarded message -
> From:
> Date: Mon, Dec 18, 2023 at 8:42 AM
> Subject: New Defects reported by Coverity Scan for Das U-Boot
> To:
>
>
> 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 470930: Uninitialized variables (UNINIT)
> /boot/bootmeth_efi.c: 465 in distro_efi_boot()
>
>
>
> *** CID 470930: Uninitialized variables (UNINIT)
> /boot/bootmeth_efi.c: 465 in distro_efi_boot()
> 459 */
> 460 if (bflow->flags & BOOTFLOWF_USE_BUILTIN_FDT) {
> 461 log_debug("Booting with built-in fdt\n");
> 462 snprintf(cmd, sizeof(cmd), "bootefi %lx", kernel);
> 463 } else {
> 464 log_debug("Booting with external fdt\n");
> >>> CID 470930: Uninitialized variables (UNINIT)
> >>> Using uninitialized value "fdt" when calling "snprintf". [Note: The
> >>> source code implementation of the function has been overridden by a
> >>> builtin model.]
> 465 snprintf(cmd, sizeof(cmd), "bootefi %lx %lx",
> kernel, fdt);
> 466 }
> 467
> 468 if (run_command(cmd, 0))
> 469 return log_msg_ret("run", -EINVAL);
> 470
>
The code in question is
if (!bootmeth_uses_network(bflow)) {
// snip
if (bflow->flags & ~BOOTFLOWF_USE_BUILTIN_FDT)
fdt = bflow->fdt_addr;
} else {
// snip
fdt = env_get_hex("fdt_addr_r", 0);
}
//snip
if (bflow->flags & BOOTFLOWF_USE_BUILTIN_FDT) {
log_debug("Booting with built-in fdt\n");
snprintf(cmd, sizeof(cmd), "bootefi %lx", kernel);
} else {
log_debug("Booting with external fdt\n");
snprintf(cmd, sizeof(cmd), "bootefi %lx %lx", kernel, fdt);
}
I am unsure in which case is fdt uninitialised.
Unless the tool is being thrown off by different version of
bflow->flags & BOOTFLOWF_USE_BUILTIN_FDT
check, I don't see the problem.
Please let me know if you see it.
Kind regards,
Shantur
>
> --
> Tom
[tom.r...@gmail.com: Fwd: New Defects reported by Coverity Scan for Das U-Boot]
Here's the latest report.
-- Forwarded message -
From:
Date: Mon, Dec 18, 2023 at 8:42 AM
Subject: New Defects reported by Coverity Scan for Das U-Boot
To:
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 470930: Uninitialized variables (UNINIT)
/boot/bootmeth_efi.c: 465 in distro_efi_boot()
*** CID 470930: Uninitialized variables (UNINIT)
/boot/bootmeth_efi.c: 465 in distro_efi_boot()
459 */
460 if (bflow->flags & BOOTFLOWF_USE_BUILTIN_FDT) {
461 log_debug("Booting with built-in fdt\n");
462 snprintf(cmd, sizeof(cmd), "bootefi %lx", kernel);
463 } else {
464 log_debug("Booting with external fdt\n");
>>> CID 470930: Uninitialized variables (UNINIT)
>>> Using uninitialized value "fdt" when calling "snprintf". [Note: The
>>> source code implementation of the function has been overridden by a builtin
>>> model.]
465 snprintf(cmd, sizeof(cmd), "bootefi %lx %lx",
kernel, fdt);
466 }
467
468 if (run_command(cmd, 0))
469 return log_msg_ret("run", -EINVAL);
470
--
Tom
signature.asc
Description: PGP signature
Re: [tom.r...@gmail.com: Fwd: New Defects reported by Coverity Scan for Das U-Boot]
On Wed, Nov 08, 2023 at 03:24:38AM +, Alexander Gendin wrote: > On Mon, Nov 06, 2023 at 03:27:52PM -0500, Tom Rini wrote: > > > Hey all, > > > > Here's the latest report. I _think_ I passed the right options to > > get_maintainer.pl such that it would only look far enough back in git to > > find the likely authors (along with listed maintainers of the files). > > > > -- Forwarded message - > > From: > > Date: Mon, Nov 6, 2023 at 2:58 PM > > Subject: New Defects reported by Coverity Scan for Das U-Boot > > To: > > > > Hi, > > > > Please find the latest report on new defect(s) introduced to Das > > U-Boot found with Coverity Scan. > > > > 13 new defect(s) introduced to Das U-Boot found with Coverity Scan. > > 5 defect(s), reported by Coverity Scan earlier, were marked fixed in > > the recent build analyzed by Coverity Scan. > > > > New defect(s) Reported-by: Coverity Scan > > Showing 13 of 13 defect(s) > > > > <...skipped...> > > *** CID 467404: Control flow issues (DEADCODE) > > /test/cmd/mbr.c: 217 in build_mbr_parts() > > 211 return 1; > > 212 strcat(cur_buf, mbr_parts_p5); > > 213 bytes_remaining -= cur_str_size; > > 214 > > 215 } > > 216 else if (num_parts > 5) > > >>> CID 467404: Control flow issues (DEADCODE) > > >>> Execution cannot reach this statement: "return 1U;". > > 217 return 1; > > 218 } > > 219 } > > 220 } > > 221 > > 222 cur_str_size = sizeof(mbr_parts_tail); > > <...skipped...> > > > > -- > > Tom > > Hi Tom, > > Thanks for the report. > > The patch to fix CID 467404 has been sent to the mailing list. Thanks for the patch, I'll make sure to grab it before the next -rc. -- Tom signature.asc Description: PGP signature
Re: [tom.r...@gmail.com: Fwd: New Defects reported by Coverity Scan for Das U-Boot]
On Wed, Nov 08, 2023 at 12:18:05AM +0100, Johan Jonker wrote:
> Hi Tom, Simon,
>
> Please have a look some comments below at 3 issues that are introduced by
> meself. ;)
Thanks for looking.
>
> On 11/6/23 21:27, Tom Rini wrote:
> > Hey all,
> >
> > Here's the latest report. I _think_ I passed the right options to
> > get_maintainer.pl such that it would only look far enough back in git to
> > find the likely authors (along with listed maintainers of the files).
> >
> > -- Forwarded message -
> > From:
> > Date: Mon, Nov 6, 2023 at 2:58 PM
> > Subject: New Defects reported by Coverity Scan for Das U-Boot
> > To:
> >
> >
> > Hi,
> >
> > Please find the latest report on new defect(s) introduced to Das
> > U-Boot found with Coverity Scan.
> >
> > 13 new defect(s) introduced to Das U-Boot found with Coverity Scan.
> > 5 defect(s), reported by Coverity Scan earlier, were marked fixed in
> > the recent build analyzed by Coverity Scan.
> >
> > New defect(s) Reported-by: Coverity Scan
> > Showing 13 of 13 defect(s)
> >
> >
> > ** CID 467411: Memory - corruptions (OVERRUN)
> >
> >
> >
>
> [..]
>
> > *** CID 467407: Uninitialized variables (UNINIT)
> > /drivers/scsi/scsi.c: 612 in do_scsi_scan_one()
> > 606
> > 607 bdesc = dev_get_uclass_plat(bdev);
> > 608 bdesc->target = id;
> > 609 bdesc->lun = lun;
> > 610 bdesc->removable = bd.removable;
> > 611 bdesc->type = bd.type;
> CID 467407: Uninitialized variables (UNINIT)
> Using uninitialized value "bd.bb".
> > 612 bdesc->bb = bd.bb;
> > 613 memcpy(>vendor, , sizeof(bd.vendor));
> > 614 memcpy(>product, , sizeof(bd.product));
> > 615 memcpy(>revision, ,
> > sizeof(bd.revision));
> > 616 if (IS_ENABLED(CONFIG_SYS_BIG_ENDIAN)) {
> > 617 ata_swap_buf_le16((u16 *)>vendor,
> > sizeof(bd.vendor) / 2);
> >
> > ** CID 467406: Memory - corruptions (OVERRUN)
> >
> >
>
> Scsi devices are not my thing.
> I'm forced to poke in drivers, because someone else is plumbing bounce buffer
> code to our block class.
OK. So I think Marek might be better able to comment on why yes, we need
bounce buffers more often than we used to, for sanity sake. But...
> Introduced by:
> [PATCH v5 4/8] rockchip: block: blk-uclass: add bounce buffer flag to blk_desc
> https://lore.kernel.org/u-boot/ee332375-8812-8e12-da1c-9973d10a4...@gmail.com/
>
> static void scsi_init_dev_desc_priv(struct blk_desc *dev_desc)
> {
> [..]
>
> #if IS_ENABLED(CONFIG_BOUNCE_BUFFER)
> dev_desc->bb = true;
> #endif/* CONFIG_BOUNCE_BUFFER */
> }
>
> [..]
> struct blk_desc bd;
>
> scsi_init_dev_desc_priv();
>
> bdesc->bb = bd.bb;
> ===
> https://www.kernel.org/doc/html/latest/dev-tools/checkpatch.html
>
> GLOBAL_INITIALISERS
>
> Global variables should not be initialized explicitly to 0 (or NULL,
> false, etc.). Your compiler (or rather your loader, which is responsible for
> zeroing out the relevant sections) automatically does it for you.
> INITIALISED_STATIC
>
> Static variables should not be initialized explicitly to zero. Your
> compiler (or rather your loader) automatically does it for you.
>
> ===
> I assumed that variables are always zeroed in the blk_desc structure.
> The value of bb only matters if IS_ENABLED(CONFIG_BOUNCE_BUFFER).
>
> For scsi:
> dev_desc->bb = IS_ENABLED(CONFIG_BOUNCE_BUFFER) ? true : false;
>
> The scsi block class worked fine for years without bounce buffer. Do all scsi
> devices need that buffer all of a sudden? What didn't work then?
>
> Please advise what is the best approach.
Popping back to do_scsi_scan_one(), 'bd' isn't a global variable, it's a
function variable. And before bounce buffers, scsi_init_dev_desc_priv()
which is called a bit earlier in the function that Coverity is talking
about here would initialize every member, as you note in your analysis.
But it doesn't set bb in the case of CONFIG_BOUNCE_BUFFER=n. A simple
fix to this would be for that function to memset everything to 0 an then
change any values it needs to set. I've done that locally and will post
and CC you for sanity testing all the same.
> ===
> There is a patch in review that changes this file. Better let that go in
> first before changing something here. What's the status?
>
> [PATCH] scsi: Forceably finish migration to DM_SCSI
> https://lore.kernel.org/u-boot/20231028005951.1187616-1-tr...@konsulko.com/
I've merged that to next this morning, but it doesn't change this part
of the code (it was removing legacy code).
> >
>
> [..]
>
> >
Re: [tom.r...@gmail.com: Fwd: New Defects reported by Coverity Scan for Das U-Boot]
On Mon, Nov 06, 2023 at 03:27:52PM -0500, Tom Rini wrote: > Hey all, > > Here's the latest report. I _think_ I passed the right options to > get_maintainer.pl such that it would only look far enough back in git to > find the likely authors (along with listed maintainers of the files). > > -- Forwarded message - > From: > Date: Mon, Nov 6, 2023 at 2:58 PM > Subject: New Defects reported by Coverity Scan for Das U-Boot > To: > > Hi, > > Please find the latest report on new defect(s) introduced to Das > U-Boot found with Coverity Scan. > > 13 new defect(s) introduced to Das U-Boot found with Coverity Scan. > 5 defect(s), reported by Coverity Scan earlier, were marked fixed in > the recent build analyzed by Coverity Scan. > > New defect(s) Reported-by: Coverity Scan > Showing 13 of 13 defect(s) > > <...skipped...> > *** CID 467404: Control flow issues (DEADCODE) > /test/cmd/mbr.c: 217 in build_mbr_parts() > 211 return 1; > 212 strcat(cur_buf, mbr_parts_p5); > 213 bytes_remaining -= cur_str_size; > 214 > 215 } > 216 else if (num_parts > 5) > >>> CID 467404: Control flow issues (DEADCODE) > >>> Execution cannot reach this statement: "return 1U;". > 217 return 1; > 218 } > 219 } > 220 } > 221 > 222 cur_str_size = sizeof(mbr_parts_tail); > <...skipped...> > > -- > Tom Hi Tom, Thanks for the report. The patch to fix CID 467404 has been sent to the mailing list. Regards, Alex --
Re: [tom.r...@gmail.com: Fwd: New Defects reported by Coverity Scan for Das U-Boot]
Hi Tom, Simon,
Please have a look some comments below at 3 issues that are introduced by
meself. ;)
On 11/6/23 21:27, Tom Rini wrote:
> Hey all,
>
> Here's the latest report. I _think_ I passed the right options to
> get_maintainer.pl such that it would only look far enough back in git to
> find the likely authors (along with listed maintainers of the files).
>
> -- Forwarded message -
> From:
> Date: Mon, Nov 6, 2023 at 2:58 PM
> Subject: New Defects reported by Coverity Scan for Das U-Boot
> To:
>
>
> Hi,
>
> Please find the latest report on new defect(s) introduced to Das
> U-Boot found with Coverity Scan.
>
> 13 new defect(s) introduced to Das U-Boot found with Coverity Scan.
> 5 defect(s), reported by Coverity Scan earlier, were marked fixed in
> the recent build analyzed by Coverity Scan.
>
> New defect(s) Reported-by: Coverity Scan
> Showing 13 of 13 defect(s)
>
>
> ** CID 467411: Memory - corruptions (OVERRUN)
>
>
>
[..]
> *** CID 467407: Uninitialized variables (UNINIT)
> /drivers/scsi/scsi.c: 612 in do_scsi_scan_one()
> 606
> 607 bdesc = dev_get_uclass_plat(bdev);
> 608 bdesc->target = id;
> 609 bdesc->lun = lun;
> 610 bdesc->removable = bd.removable;
> 611 bdesc->type = bd.type;
CID 467407: Uninitialized variables (UNINIT)
Using uninitialized value "bd.bb".
> 612 bdesc->bb = bd.bb;
> 613 memcpy(>vendor, , sizeof(bd.vendor));
> 614 memcpy(>product, , sizeof(bd.product));
> 615 memcpy(>revision, , sizeof(bd.revision));
> 616 if (IS_ENABLED(CONFIG_SYS_BIG_ENDIAN)) {
> 617 ata_swap_buf_le16((u16 *)>vendor,
> sizeof(bd.vendor) / 2);
>
> ** CID 467406: Memory - corruptions (OVERRUN)
>
>
Scsi devices are not my thing.
I'm forced to poke in drivers, because someone else is plumbing bounce buffer
code to our block class.
Introduced by:
[PATCH v5 4/8] rockchip: block: blk-uclass: add bounce buffer flag to blk_desc
https://lore.kernel.org/u-boot/ee332375-8812-8e12-da1c-9973d10a4...@gmail.com/
static void scsi_init_dev_desc_priv(struct blk_desc *dev_desc)
{
[..]
#if IS_ENABLED(CONFIG_BOUNCE_BUFFER)
dev_desc->bb = true;
#endif /* CONFIG_BOUNCE_BUFFER */
}
[..]
struct blk_desc bd;
scsi_init_dev_desc_priv();
bdesc->bb = bd.bb;
===
https://www.kernel.org/doc/html/latest/dev-tools/checkpatch.html
GLOBAL_INITIALISERS
Global variables should not be initialized explicitly to 0 (or NULL, false,
etc.). Your compiler (or rather your loader, which is responsible for zeroing
out the relevant sections) automatically does it for you.
INITIALISED_STATIC
Static variables should not be initialized explicitly to zero. Your
compiler (or rather your loader) automatically does it for you.
===
I assumed that variables are always zeroed in the blk_desc structure.
The value of bb only matters if IS_ENABLED(CONFIG_BOUNCE_BUFFER).
For scsi:
dev_desc->bb = IS_ENABLED(CONFIG_BOUNCE_BUFFER) ? true : false;
The scsi block class worked fine for years without bounce buffer. Do all scsi
devices need that buffer all of a sudden? What didn't work then?
Please advise what is the best approach.
===
There is a patch in review that changes this file. Better let that go in first
before changing something here. What's the status?
[PATCH] scsi: Forceably finish migration to DM_SCSI
https://lore.kernel.org/u-boot/20231028005951.1187616-1-tr...@konsulko.com/
>
[..]
>
> *** CID 467402:(CHECKED_RETURN)
> /drivers/block/rkmtd.c: 737 in rkmtd_init_plat()
> 731
> 732 debug("starting_lba : %llu\n",
> le64_to_cpu(plat->gpt_e->starting_lba));
> 733 debug("ending_lba : %llu\n",
> le64_to_cpu(plat->gpt_e->ending_lba));
> 734
> 735 memcpy(plat->gpt_e->partition_type_guid.b,
> _basic_data_guid, 16);
> 736
CID 467402:(CHECKED_RETURN)
Calling "uuid_str_to_bin" without checking return value (as is done
elsewhere 9 out of 11 times).
> 737 uuid_str_to_bin(plat->uuid_part_str,
> plat->gpt_e->unique_partition_guid.b,
> 738 UUID_STR_FORMAT_GUID);
Comment 2:
gen_rand_uuid_str(plat->uuid_disk_str, UUID_STR_FORMAT_GUID);
uuid_str_to_bin(plat->uuid_part_str,
plat->gpt_e->unique_partition_guid.b,
UUID_STR_FORMAT_GUID);
The function uuid_str_to_bin() gets a string from gen_rand_uuid_str() which is
guarantied correct.
Checking the output is unnecessary.
if (!uuid_str_valid(uuid_str)) {
return
Re: [tom.r...@gmail.com: Fwd: New Defects reported by Coverity Scan for Das U-Boot]
Hi Tom,
Thanks for the report.
Eddie, can you please check the TCG related ones?
Thanks
/Ilias
On Mon, 6 Nov 2023 at 22:27, Tom Rini wrote:
>
> Hey all,
>
> Here's the latest report. I _think_ I passed the right options to
> get_maintainer.pl such that it would only look far enough back in git to
> find the likely authors (along with listed maintainers of the files).
>
> -- Forwarded message -
> From:
> Date: Mon, Nov 6, 2023 at 2:58 PM
> Subject: New Defects reported by Coverity Scan for Das U-Boot
> To:
>
>
> Hi,
>
> Please find the latest report on new defect(s) introduced to Das
> U-Boot found with Coverity Scan.
>
> 13 new defect(s) introduced to Das U-Boot found with Coverity Scan.
> 5 defect(s), reported by Coverity Scan earlier, were marked fixed in
> the recent build analyzed by Coverity Scan.
>
> New defect(s) Reported-by: Coverity Scan
> Showing 13 of 13 defect(s)
>
>
> ** CID 467411: Memory - corruptions (OVERRUN)
>
>
>
> *** CID 467411: Memory - corruptions (OVERRUN)
> /lib/efi_loader/efi_tcg2.c: 1395 in efi_tcg2_measure_efi_app_invocation()
> 1389
> 1390ret = tcg2_measure_gpt_data(dev, handle);
> 1391if (ret != EFI_SUCCESS)
> 1392goto out;
> 1393
> 1394for (pcr_index = 0; pcr_index <= 7; pcr_index++) {
> >>> CID 467411: Memory - corruptions (OVERRUN)
> >>> Overrunning buffer pointed to by "(u8 *)" of 4 bytes by passing
> >>> it to a function which accesses it at byte offset 63.
> 1395ret = measure_event(dev, pcr_index, EV_SEPARATOR,
> 1396sizeof(event), (u8 *));
> 1397if (ret != EFI_SUCCESS)
> 1398goto out;
> 1399}
> 1400
>
> ** CID 467410:(TAINTED_SCALAR)
>
>
>
> *** CID 467410:(TAINTED_SCALAR)
> /lib/efi_loader/efi_tcg2.c: 1385 in efi_tcg2_measure_efi_app_invocation()
> 1379(u8 *)EFI_CALLING_EFI_APPLICATION);
> 1380if (ret != EFI_SUCCESS)
> 1381goto out;
> 1382
> 1383entry = (struct smbios_entry *)find_smbios_table();
> 1384if (entry) {
> >>> CID 467410:(TAINTED_SCALAR)
> >>> Passing tainted expression "entry->struct_table_length" to
> >>> "tcg2_measure_smbios", which uses it as an offset.
> 1385ret = tcg2_measure_smbios(dev, entry);
> 1386if (ret != EFI_SUCCESS)
> 1387goto out;
> 1388}
> 1389
> 1390ret = tcg2_measure_gpt_data(dev, handle);
> /lib/efi_loader/efi_tcg2.c: 1385 in efi_tcg2_measure_efi_app_invocation()
> 1379(u8 *)EFI_CALLING_EFI_APPLICATION);
> 1380if (ret != EFI_SUCCESS)
> 1381goto out;
> 1382
> 1383entry = (struct smbios_entry *)find_smbios_table();
> 1384if (entry) {
> >>> CID 467410:(TAINTED_SCALAR)
> >>> Passing tainted expression "entry->struct_count" to
> >>> "tcg2_measure_smbios", which uses it as a loop boundary.
> 1385ret = tcg2_measure_smbios(dev, entry);
> 1386if (ret != EFI_SUCCESS)
> 1387goto out;
> 1388}
> 1389
> 1390ret = tcg2_measure_gpt_data(dev, handle);
>
> ** CID 467409: Uninitialized variables (UNINIT)
>
>
>
> *** CID 467409: Uninitialized variables (UNINIT)
> /test/boot/measurement.c: 48 in measure()
> 42 for (i = 0; i < size; ++i) {
> 43 kernel[i] = 0xf0 | (i & 0xf);
> 44 initrd[i] = (i & 0xf0) | 0xf;
> 45 images.ft_addr[i] = i & 0xff;
> 46 }
> 47
> >>> CID 467409: Uninitialized variables (UNINIT)
> >>> Using uninitialized value "images.os.os" when calling "bootm_measure".
> 48 ut_assertok(bootm_measure());
> 49
> 50 free(images.ft_addr);
> 51 free(initrd);
> 52 free(kernel);
> 53
>
> ** CID 467408: Insecure data handling (TAINTED_SCALAR)
>
>
>
> *** CID 467408: Insecure data handling (TAINTED_SCALAR)
> /boot/bootm.c: 826 in do_bootm_states()
> 820 env_set_hex("initrd_end", images->initrd_end);
> 821 }
> 822 }
> 823 #endif
> 824 #if CONFIG_IS_ENABLED(OF_LIBFDT) && defined(CONFIG_LMB)
> 825 if (!ret && (states & BOOTM_STATE_FDT)) {
> >>> CID 467408: Insecure data handling (TAINTED_SCALAR)
> >>> Passing tainted expression "*images->ft_addr" to
> >>>
[tom.r...@gmail.com: Fwd: New Defects reported by Coverity Scan for Das U-Boot]
Hey all,
Here's the latest report. I _think_ I passed the right options to
get_maintainer.pl such that it would only look far enough back in git to
find the likely authors (along with listed maintainers of the files).
-- Forwarded message -
From:
Date: Mon, Nov 6, 2023 at 2:58 PM
Subject: New Defects reported by Coverity Scan for Das U-Boot
To:
Hi,
Please find the latest report on new defect(s) introduced to Das
U-Boot found with Coverity Scan.
13 new defect(s) introduced to Das U-Boot found with Coverity Scan.
5 defect(s), reported by Coverity Scan earlier, were marked fixed in
the recent build analyzed by Coverity Scan.
New defect(s) Reported-by: Coverity Scan
Showing 13 of 13 defect(s)
** CID 467411: Memory - corruptions (OVERRUN)
*** CID 467411: Memory - corruptions (OVERRUN)
/lib/efi_loader/efi_tcg2.c: 1395 in efi_tcg2_measure_efi_app_invocation()
1389
1390ret = tcg2_measure_gpt_data(dev, handle);
1391if (ret != EFI_SUCCESS)
1392goto out;
1393
1394for (pcr_index = 0; pcr_index <= 7; pcr_index++) {
>>> CID 467411: Memory - corruptions (OVERRUN)
>>> Overrunning buffer pointed to by "(u8 *)" of 4 bytes by passing
>>> it to a function which accesses it at byte offset 63.
1395ret = measure_event(dev, pcr_index, EV_SEPARATOR,
1396sizeof(event), (u8 *));
1397if (ret != EFI_SUCCESS)
1398goto out;
1399}
1400
** CID 467410:(TAINTED_SCALAR)
*** CID 467410:(TAINTED_SCALAR)
/lib/efi_loader/efi_tcg2.c: 1385 in efi_tcg2_measure_efi_app_invocation()
1379(u8 *)EFI_CALLING_EFI_APPLICATION);
1380if (ret != EFI_SUCCESS)
1381goto out;
1382
1383entry = (struct smbios_entry *)find_smbios_table();
1384if (entry) {
>>> CID 467410:(TAINTED_SCALAR)
>>> Passing tainted expression "entry->struct_table_length" to
>>> "tcg2_measure_smbios", which uses it as an offset.
1385ret = tcg2_measure_smbios(dev, entry);
1386if (ret != EFI_SUCCESS)
1387goto out;
1388}
1389
1390ret = tcg2_measure_gpt_data(dev, handle);
/lib/efi_loader/efi_tcg2.c: 1385 in efi_tcg2_measure_efi_app_invocation()
1379(u8 *)EFI_CALLING_EFI_APPLICATION);
1380if (ret != EFI_SUCCESS)
1381goto out;
1382
1383entry = (struct smbios_entry *)find_smbios_table();
1384if (entry) {
>>> CID 467410:(TAINTED_SCALAR)
>>> Passing tainted expression "entry->struct_count" to
>>> "tcg2_measure_smbios", which uses it as a loop boundary.
1385ret = tcg2_measure_smbios(dev, entry);
1386if (ret != EFI_SUCCESS)
1387goto out;
1388}
1389
1390ret = tcg2_measure_gpt_data(dev, handle);
** CID 467409: Uninitialized variables (UNINIT)
*** CID 467409: Uninitialized variables (UNINIT)
/test/boot/measurement.c: 48 in measure()
42 for (i = 0; i < size; ++i) {
43 kernel[i] = 0xf0 | (i & 0xf);
44 initrd[i] = (i & 0xf0) | 0xf;
45 images.ft_addr[i] = i & 0xff;
46 }
47
>>> CID 467409: Uninitialized variables (UNINIT)
>>> Using uninitialized value "images.os.os" when calling "bootm_measure".
48 ut_assertok(bootm_measure());
49
50 free(images.ft_addr);
51 free(initrd);
52 free(kernel);
53
** CID 467408: Insecure data handling (TAINTED_SCALAR)
*** CID 467408: Insecure data handling (TAINTED_SCALAR)
/boot/bootm.c: 826 in do_bootm_states()
820 env_set_hex("initrd_end", images->initrd_end);
821 }
822 }
823 #endif
824 #if CONFIG_IS_ENABLED(OF_LIBFDT) && defined(CONFIG_LMB)
825 if (!ret && (states & BOOTM_STATE_FDT)) {
>>> CID 467408: Insecure data handling (TAINTED_SCALAR)
>>> Passing tainted expression "*images->ft_addr" to
>>> "boot_fdt_add_mem_rsv_regions", which uses it as an offset.
826 boot_fdt_add_mem_rsv_regions(>lmb,
images->ft_addr);
827 ret = boot_relocate_fdt(>lmb, >ft_addr,
828 >ft_len);
829 }
830 #endif
831
** CID 467407: Uninitialized variables (UNINIT)
/drivers/scsi/scsi.c: 612 in
Re: Fwd: New Defects reported by Coverity Scan for Das U-Boot
Hi Tom, > > > > > > > > > > *** CID 464361: Control flow issues (DEADCODE) > > > > > /drivers/firmware/arm-ffa/arm-ffa-uclass.c: 148 in > > > > > ffa_print_error_log() > > > > > 142 > > > > > 143 if (ffa_id < FFA_FIRST_ID || ffa_id > FFA_LAST_ID) > > > > > 144 return -EINVAL; > > > > > 145 > > > > > 146 abi_idx = FFA_ID_TO_ERRMAP_ID(ffa_id); > > > > > 147 if (abi_idx < 0 || abi_idx >= FFA_ERRMAP_COUNT) > > > > > >>> CID 464361: Control flow issues (DEADCODE) > > > > > >>> Execution cannot reach this statement: "return -22;". > > > > > 148 return -EINVAL; > > > > > > > > This is a false positive. > > > > > > > > abi_idx value could end up matching this condition "(abi_idx < 0 || > > > > abi_idx >= FFA_ERRMAP_COUNT)". > > > > > > > > This happens when ffa_id value is above the allowed bounds. Example: > > > > when ffa_id is 0x50 or 0x80 > > > > > > > > ffa_print_error_log(0x50, ...); /* exceeding lower bound */ > > > > ffa_print_error_log(0x80, ...); /* exceeding upper bound */ > > > > > > > > In these cases "return -EINVAL;" is executed. > > > > > > So those invalid values aren't caught by the previous check that ffa_id > > > falls within FFA_FIRST_ID to FFA_LAST_ID ? > > > > I had a closer look at that and I agree that the deadcode defect is > > legitimate. > > I already provided a fix [1]. > > > > [1]: > > https://lore.kernel.org/all/20231020131533.239591-1-abdellatif.elkhl...@arm.com/ > > Ah thanks. I had seen that posted but not put that together with this > email and assumed it was addressing something you hadn't talked about > here because you agreed with it being an issue. I will pick up the > above patch soon then. Thank you very much. Cheers, Abdellatif
Re: Fwd: New Defects reported by Coverity Scan for Das U-Boot
On Wed, Oct 25, 2023 at 04:12:37PM +0100, Abdellatif El Khlifi wrote: > Hi Tom, > > > > > > > > > *** CID 464361: Control flow issues (DEADCODE) > > > > /drivers/firmware/arm-ffa/arm-ffa-uclass.c: 148 in ffa_print_error_log() > > > > 142 > > > > 143 if (ffa_id < FFA_FIRST_ID || ffa_id > FFA_LAST_ID) > > > > 144 return -EINVAL; > > > > 145 > > > > 146 abi_idx = FFA_ID_TO_ERRMAP_ID(ffa_id); > > > > 147 if (abi_idx < 0 || abi_idx >= FFA_ERRMAP_COUNT) > > > > >>> CID 464361: Control flow issues (DEADCODE) > > > > >>> Execution cannot reach this statement: "return -22;". > > > > 148 return -EINVAL; > > > > > > This is a false positive. > > > > > > abi_idx value could end up matching this condition "(abi_idx < 0 || > > > abi_idx >= FFA_ERRMAP_COUNT)". > > > > > > This happens when ffa_id value is above the allowed bounds. Example: when > > > ffa_id is 0x50 or 0x80 > > > > > > ffa_print_error_log(0x50, ...); /* exceeding lower bound */ > > > ffa_print_error_log(0x80, ...); /* exceeding upper bound */ > > > > > > In these cases "return -EINVAL;" is executed. > > > > So those invalid values aren't caught by the previous check that ffa_id > > falls within FFA_FIRST_ID to FFA_LAST_ID ? > > I had a closer look at that and I agree that the deadcode defect is > legitimate. > I already provided a fix [1]. > > [1]: > https://lore.kernel.org/all/20231020131533.239591-1-abdellatif.elkhl...@arm.com/ Ah thanks. I had seen that posted but not put that together with this email and assumed it was addressing something you hadn't talked about here because you agreed with it being an issue. I will pick up the above patch soon then. -- Tom signature.asc Description: PGP signature
Re: Fwd: New Defects reported by Coverity Scan for Das U-Boot
Hi Tom,
> > >
> > > *** CID 464361: Control flow issues (DEADCODE)
> > > /drivers/firmware/arm-ffa/arm-ffa-uclass.c: 148 in ffa_print_error_log()
> > > 142
> > > 143 if (ffa_id < FFA_FIRST_ID || ffa_id > FFA_LAST_ID)
> > > 144 return -EINVAL;
> > > 145
> > > 146 abi_idx = FFA_ID_TO_ERRMAP_ID(ffa_id);
> > > 147 if (abi_idx < 0 || abi_idx >= FFA_ERRMAP_COUNT)
> > > >>> CID 464361: Control flow issues (DEADCODE)
> > > >>> Execution cannot reach this statement: "return -22;".
> > > 148 return -EINVAL;
> >
> > This is a false positive.
> >
> > abi_idx value could end up matching this condition "(abi_idx < 0 ||
> > abi_idx >= FFA_ERRMAP_COUNT)".
> >
> > This happens when ffa_id value is above the allowed bounds. Example: when
> > ffa_id is 0x50 or 0x80
> >
> > ffa_print_error_log(0x50, ...); /* exceeding lower bound */
> > ffa_print_error_log(0x80, ...); /* exceeding upper bound */
> >
> > In these cases "return -EINVAL;" is executed.
>
> So those invalid values aren't caught by the previous check that ffa_id
> falls within FFA_FIRST_ID to FFA_LAST_ID ?
I had a closer look at that and I agree that the deadcode defect is legitimate.
I already provided a fix [1].
[1]:
https://lore.kernel.org/all/20231020131533.239591-1-abdellatif.elkhl...@arm.com/
>
> > > ...
> > >
> > > *** CID 464360: Control flow issues (NO_EFFECT)
> > > /drivers/firmware/arm-ffa/arm-ffa-uclass.c: 207 in ffa_get_version_hdlr()
> > > 201 major = GET_FFA_MAJOR_VERSION(res.a0);
> > > 202 minor = GET_FFA_MINOR_VERSION(res.a0);
> > > 203
> > > 204 log_debug("FF-A driver %d.%d\nFF-A framework %d.%d\n",
> > > 205 FFA_MAJOR_VERSION, FFA_MINOR_VERSION, major,
> > > minor);
> > > 206
> > > >>> CID 464360: Control flow issues (NO_EFFECT)
> > > >>> This greater-than-or-equal-to-zero comparison of an unsigned
> > > >>> value is always true. "minor >= 0".
> > > 207 if (major == FFA_MAJOR_VERSION && minor >=
> > > FFA_MINOR_VERSION) {
> >
> > Providing the facts that:
> >
> > #define FFA_MINOR_VERSION (0)
> > u16 minor;
> >
> > Yes, currently this condition is always true: minor >= FFA_MINOR_VERSION
> >
> > However, we might upgrade FFA_MINOR_VERSION in the future. If we remove the
> > "minor >= FFA_MINOR_VERSION" ,
> > non compatible versions could pass which we don't want.
> >
> > To keep this code scalable, I think it's better to keep this condition.
>
> OK, thanks this makes sense as an intentional change for future sanity
> checking.
>
> > >
> > > *** CID 464359:(PASS_BY_VALUE)
> > > /drivers/firmware/arm-ffa/arm-ffa-uclass.c: 168 in invoke_ffa_fn()
> > > 162 * @args: FF-A ABI arguments to be copied to Xn registers
> > > 163 * @res: FF-A ABI return data to be copied from Xn registers
> > > 164 *
> > > 165 * Calls low level SMC implementation.
> > > 166 * This function should be implemented by the user driver.
> > > 167 */
> > > >>> CID 464359:(PASS_BY_VALUE)
> > > >>> Passing parameter args of type "ffa_value_t" (size 144 bytes) by
> > > >>> value, which exceeds the low threshold of 128 bytes.
> > > 168 void __weak invoke_ffa_fn(ffa_value_t args, ffa_value_t *res)
> >
> > We are using invoke_ffa_fn with the same arguments as in linux. The aim is
> > to use the same interfaces as in the Linux FF-A
> > driver to make porting code easier.
> >
> > In Linux, args is passed by value [1].
> > ffa_value_t is a structure with 18 "unsigned long" fields. So, the size is
> > fixed.
> >
> > [1]: invoke_ffa_fn arguments in the Linux FF-A driver
> >
> > https://elixir.bootlin.com/linux/v6.6-rc6/source/drivers/firmware/arm_ffa/driver.c#L115
> > https://elixir.bootlin.com/linux/v6.6-rc6/source/drivers/firmware/arm_ffa/driver.c#L54
> > https://elixir.bootlin.com/linux/v6.6-rc6/source/drivers/firmware/arm_ffa/common.h#L15
> >
> > [2]: include/linux/arm-smccc.h
>
> So this is intentional, OK.
>
> >
> > > 169 {
> > > 170 }
> > > 171
> > > 172 /**
> > > 173 * ffa_get_version_hdlr() - FFA_VERSION handler function
> > > /drivers/firmware/arm-ffa/ffa-emul-uclass.c: 673 in invoke_ffa_fn()
> > > 667 * invoke_ffa_fn() - SMC wrapper
> > > 668 * @args: FF-A ABI arguments to be copied to Xn registers
> > > 669 * @res: FF-A ABI return data to be copied from Xn registers
> > > 670 *
> > > 671 * Calls the emulated SMC call.
> > > 672 */
> > > >>> CID 464359:(PASS_BY_VALUE)
> > > >>> Passing parameter args of type "ffa_value_t" (size 144 bytes) by
Re: Fwd: New Defects reported by Coverity Scan for Das U-Boot
On Fri, Oct 20, 2023 at 12:57:47PM +0100, Abdellatif El Khlifi wrote:
> Hi Tom,
>
> >
> > *** CID 464361: Control flow issues (DEADCODE)
> > /drivers/firmware/arm-ffa/arm-ffa-uclass.c: 148 in ffa_print_error_log()
> > 142
> > 143 if (ffa_id < FFA_FIRST_ID || ffa_id > FFA_LAST_ID)
> > 144 return -EINVAL;
> > 145
> > 146 abi_idx = FFA_ID_TO_ERRMAP_ID(ffa_id);
> > 147 if (abi_idx < 0 || abi_idx >= FFA_ERRMAP_COUNT)
> > >>> CID 464361: Control flow issues (DEADCODE)
> > >>> Execution cannot reach this statement: "return -22;".
> > 148 return -EINVAL;
>
> This is a false positive.
>
> abi_idx value could end up matching this condition "(abi_idx < 0 || abi_idx
> >= FFA_ERRMAP_COUNT)".
>
> This happens when ffa_id value is above the allowed bounds. Example: when
> ffa_id is 0x50 or 0x80
>
> ffa_print_error_log(0x50, ...); /* exceeding lower bound */
> ffa_print_error_log(0x80, ...); /* exceeding upper bound */
>
> In these cases "return -EINVAL;" is executed.
So those invalid values aren't caught by the previous check that ffa_id
falls within FFA_FIRST_ID to FFA_LAST_ID ?
> > ...
> >
> > *** CID 464360: Control flow issues (NO_EFFECT)
> > /drivers/firmware/arm-ffa/arm-ffa-uclass.c: 207 in ffa_get_version_hdlr()
> > 201 major = GET_FFA_MAJOR_VERSION(res.a0);
> > 202 minor = GET_FFA_MINOR_VERSION(res.a0);
> > 203
> > 204 log_debug("FF-A driver %d.%d\nFF-A framework %d.%d\n",
> > 205 FFA_MAJOR_VERSION, FFA_MINOR_VERSION, major,
> > minor);
> > 206
> > >>> CID 464360: Control flow issues (NO_EFFECT)
> > >>> This greater-than-or-equal-to-zero comparison of an unsigned value
> > >>> is always true. "minor >= 0".
> > 207 if (major == FFA_MAJOR_VERSION && minor >=
> > FFA_MINOR_VERSION) {
>
> Providing the facts that:
>
> #define FFA_MINOR_VERSION (0)
> u16 minor;
>
> Yes, currently this condition is always true: minor >= FFA_MINOR_VERSION
>
> However, we might upgrade FFA_MINOR_VERSION in the future. If we remove the
> "minor >= FFA_MINOR_VERSION" ,
> non compatible versions could pass which we don't want.
>
> To keep this code scalable, I think it's better to keep this condition.
OK, thanks this makes sense as an intentional change for future sanity
checking.
> >
> > *** CID 464359:(PASS_BY_VALUE)
> > /drivers/firmware/arm-ffa/arm-ffa-uclass.c: 168 in invoke_ffa_fn()
> > 162 * @args: FF-A ABI arguments to be copied to Xn registers
> > 163 * @res: FF-A ABI return data to be copied from Xn registers
> > 164 *
> > 165 * Calls low level SMC implementation.
> > 166 * This function should be implemented by the user driver.
> > 167 */
> > >>> CID 464359:(PASS_BY_VALUE)
> > >>> Passing parameter args of type "ffa_value_t" (size 144 bytes) by
> > >>> value, which exceeds the low threshold of 128 bytes.
> > 168 void __weak invoke_ffa_fn(ffa_value_t args, ffa_value_t *res)
>
> We are using invoke_ffa_fn with the same arguments as in linux. The aim is to
> use the same interfaces as in the Linux FF-A
> driver to make porting code easier.
>
> In Linux, args is passed by value [1].
> ffa_value_t is a structure with 18 "unsigned long" fields. So, the size is
> fixed.
>
> [1]: invoke_ffa_fn arguments in the Linux FF-A driver
>
> https://elixir.bootlin.com/linux/v6.6-rc6/source/drivers/firmware/arm_ffa/driver.c#L115
> https://elixir.bootlin.com/linux/v6.6-rc6/source/drivers/firmware/arm_ffa/driver.c#L54
> https://elixir.bootlin.com/linux/v6.6-rc6/source/drivers/firmware/arm_ffa/common.h#L15
>
> [2]: include/linux/arm-smccc.h
So this is intentional, OK.
>
> > 169 {
> > 170 }
> > 171
> > 172 /**
> > 173 * ffa_get_version_hdlr() - FFA_VERSION handler function
> > /drivers/firmware/arm-ffa/ffa-emul-uclass.c: 673 in invoke_ffa_fn()
> > 667 * invoke_ffa_fn() - SMC wrapper
> > 668 * @args: FF-A ABI arguments to be copied to Xn registers
> > 669 * @res: FF-A ABI return data to be copied from Xn registers
> > 670 *
> > 671 * Calls the emulated SMC call.
> > 672 */
> > >>> CID 464359:(PASS_BY_VALUE)
> > >>> Passing parameter args of type "ffa_value_t" (size 144 bytes) by
> > >>> value, which exceeds the low threshold of 128 bytes.
> > 673 void invoke_ffa_fn(ffa_value_t args, ffa_value_t *res)
>
> Same feedback as above.
Thanks. I'll update the last 3 CIDs shortly.
--
Tom
signature.asc
Description: PGP signature
Re: [tom.r...@gmail.com: Fwd: New Defects reported by Coverity Scan for Das U-Boot]
On Tue, Oct 24, 2023 at 08:35:58PM +0530, Sughosh Ganu wrote:
> hi Tom,
>
> On Tue, 24 Oct 2023 at 06:48, Tom Rini wrote:
> >
> > Here's the latest report
> >
> > -- Forwarded message -
> > From:
> > Date: Mon, Oct 23, 2023 at 4:40 PM
> > Subject: New Defects reported by Coverity Scan for Das U-Boot
> > To:
> >
> >
> > Hi,
> >
> > Please find the latest report on new defect(s) introduced to Das
> > U-Boot found with Coverity Scan.
> >
> > 16 new defect(s) introduced to Das U-Boot found with Coverity Scan.
> > 6 defect(s), reported by Coverity Scan earlier, were marked fixed in
> > the recent build analyzed by Coverity Scan.
> >
> > New defect(s) Reported-by: Coverity Scan
> > Showing 16 of 16 defect(s)
> >
>
>
>
> >
> > ** CID 467053:(RESOURCE_LEAK)
> > /tools/mkeficapsule.c: 859 in dump_capsule_contents()
> > /tools/mkeficapsule.c: 859 in dump_capsule_contents()
> >
> >
> >
> > *** CID 467053:(RESOURCE_LEAK)
> > /tools/mkeficapsule.c: 859 in dump_capsule_contents()
> > 853 empty_capsule_dump(ptr);
> > 854 } else {
> > 855 fprintf(stderr, "Unable to decode the capsule
> > file: %s\n",
> > 856 capsule_file);
> > 857 exit(EXIT_FAILURE);
> > 858 }
> > >>> CID 467053:(RESOURCE_LEAK)
> > >>> Variable "ptr" going out of scope leaks the storage it points to.
> > 859 }
> > 860
> > 861 /**
> > 862 * main - main entry function of mkeficapsule
> > 863 * @argc: Number of arguments
> > 864 * @argv: Array of pointers to arguments
> > /tools/mkeficapsule.c: 859 in dump_capsule_contents()
> > 853 empty_capsule_dump(ptr);
> > 854 } else {
> > 855 fprintf(stderr, "Unable to decode the capsule
> > file: %s\n",
> > 856 capsule_file);
> > 857 exit(EXIT_FAILURE);
> > 858 }
> > >>> CID 467053:(RESOURCE_LEAK)
> > >>> Variable "ptr" going out of scope leaks the storage it points to.
> > 859 }
> > 860
> > 861 /**
> > 862 * main - main entry function of mkeficapsule
> > 863 * @argc: Number of arguments
> > 864 * @argv: Array of pointers to arguments
> >
>
>
>
> > ** CID 467045: Resource leaks (RESOURCE_LEAK)
> > /tools/mkeficapsule.c: 859 in dump_capsule_contents()
> >
> >
> >
> > *** CID 467045: Resource leaks (RESOURCE_LEAK)
> > /tools/mkeficapsule.c: 859 in dump_capsule_contents()
> > 853 empty_capsule_dump(ptr);
> > 854 } else {
> > 855 fprintf(stderr, "Unable to decode the capsule
> > file: %s\n",
> > 856 capsule_file);
> > 857 exit(EXIT_FAILURE);
> > 858 }
> > >>> CID 467045: Resource leaks (RESOURCE_LEAK)
> > >>> Handle variable "fd" going out of scope leaks the handle.
> > 859 }
> > 860
> > 861 /**
> > 862 * main - main entry function of mkeficapsule
> > 863 * @argc: Number of arguments
> > 864 * @argv: Array of pointers to arguments
> >
> >
>
> Both the pointer and file descriptor are not being freed since the
> process exits once the dump_capaule_contents() function returns. These
> can be marked as false positives. Thanks.
I would say that's "intentional" rather than false positive (and perhaps
a bad example) but indeed not a fatal problem. Thanks for checking.
--
Tom
signature.asc
Description: PGP signature
Re: [tom.r...@gmail.com: Fwd: New Defects reported by Coverity Scan for Das U-Boot]
hi Tom,
On Tue, 24 Oct 2023 at 06:48, Tom Rini wrote:
>
> Here's the latest report
>
> -- Forwarded message -
> From:
> Date: Mon, Oct 23, 2023 at 4:40 PM
> Subject: New Defects reported by Coverity Scan for Das U-Boot
> To:
>
>
> Hi,
>
> Please find the latest report on new defect(s) introduced to Das
> U-Boot found with Coverity Scan.
>
> 16 new defect(s) introduced to Das U-Boot found with Coverity Scan.
> 6 defect(s), reported by Coverity Scan earlier, were marked fixed in
> the recent build analyzed by Coverity Scan.
>
> New defect(s) Reported-by: Coverity Scan
> Showing 16 of 16 defect(s)
>
>
> ** CID 467053:(RESOURCE_LEAK)
> /tools/mkeficapsule.c: 859 in dump_capsule_contents()
> /tools/mkeficapsule.c: 859 in dump_capsule_contents()
>
>
>
> *** CID 467053:(RESOURCE_LEAK)
> /tools/mkeficapsule.c: 859 in dump_capsule_contents()
> 853 empty_capsule_dump(ptr);
> 854 } else {
> 855 fprintf(stderr, "Unable to decode the capsule
> file: %s\n",
> 856 capsule_file);
> 857 exit(EXIT_FAILURE);
> 858 }
> >>> CID 467053:(RESOURCE_LEAK)
> >>> Variable "ptr" going out of scope leaks the storage it points to.
> 859 }
> 860
> 861 /**
> 862 * main - main entry function of mkeficapsule
> 863 * @argc: Number of arguments
> 864 * @argv: Array of pointers to arguments
> /tools/mkeficapsule.c: 859 in dump_capsule_contents()
> 853 empty_capsule_dump(ptr);
> 854 } else {
> 855 fprintf(stderr, "Unable to decode the capsule
> file: %s\n",
> 856 capsule_file);
> 857 exit(EXIT_FAILURE);
> 858 }
> >>> CID 467053:(RESOURCE_LEAK)
> >>> Variable "ptr" going out of scope leaks the storage it points to.
> 859 }
> 860
> 861 /**
> 862 * main - main entry function of mkeficapsule
> 863 * @argc: Number of arguments
> 864 * @argv: Array of pointers to arguments
>
> ** CID 467045: Resource leaks (RESOURCE_LEAK)
> /tools/mkeficapsule.c: 859 in dump_capsule_contents()
>
>
>
> *** CID 467045: Resource leaks (RESOURCE_LEAK)
> /tools/mkeficapsule.c: 859 in dump_capsule_contents()
> 853 empty_capsule_dump(ptr);
> 854 } else {
> 855 fprintf(stderr, "Unable to decode the capsule
> file: %s\n",
> 856 capsule_file);
> 857 exit(EXIT_FAILURE);
> 858 }
> >>> CID 467045: Resource leaks (RESOURCE_LEAK)
> >>> Handle variable "fd" going out of scope leaks the handle.
> 859 }
> 860
> 861 /**
> 862 * main - main entry function of mkeficapsule
> 863 * @argc: Number of arguments
> 864 * @argv: Array of pointers to arguments
>
>
Both the pointer and file descriptor are not being freed since the
process exits once the dump_capaule_contents() function returns. These
can be marked as false positives. Thanks.
-sughosh
[tom.r...@gmail.com: Fwd: New Defects reported by Coverity Scan for Das U-Boot]
Here's the latest report
-- Forwarded message -
From:
Date: Mon, Oct 23, 2023 at 4:40 PM
Subject: New Defects reported by Coverity Scan for Das U-Boot
To:
Hi,
Please find the latest report on new defect(s) introduced to Das
U-Boot found with Coverity Scan.
16 new defect(s) introduced to Das U-Boot found with Coverity Scan.
6 defect(s), reported by Coverity Scan earlier, were marked fixed in
the recent build analyzed by Coverity Scan.
New defect(s) Reported-by: Coverity Scan
Showing 16 of 16 defect(s)
** CID 467060:(TAINTED_SCALAR)
*** CID 467060:(TAINTED_SCALAR)
/boot/bootmeth_cros.c: 184 in scan_part()
178 if (ret != num_blks) {
179 free(hdr);
180 return log_msg_ret("inf", -EIO);
181 }
182
183 if (memcmp(VB2_KEYBLOCK_MAGIC, hdr->magic,
VB2_KEYBLOCK_MAGIC_SIZE)) {
>>> CID 467060:(TAINTED_SCALAR)
>>> Passing tainted expression "*hdr" to "dlfree", which uses it as an
>>> offset.
184 free(hdr);
185 log_debug("no magic\n");
186 return -ENOENT;
187 }
188
189 *hdrp = hdr;
/boot/bootmeth_cros.c: 179 in scan_part()
173 blk->name, (ulong)info->start, num_blks);
174 hdr = memalign(SZ_1K, PROBE_SIZE);
175 if (!hdr)
176 return log_msg_ret("hdr", -ENOMEM);
177 ret = blk_read(blk, info->start, num_blks, hdr);
178 if (ret != num_blks) {
>>> CID 467060:(TAINTED_SCALAR)
>>> Passing tainted expression "*hdr" to "dlfree", which uses it as an
>>> offset.
179 free(hdr);
180 return log_msg_ret("inf", -EIO);
181 }
182
183 if (memcmp(VB2_KEYBLOCK_MAGIC, hdr->magic,
VB2_KEYBLOCK_MAGIC_SIZE)) {
184 free(hdr);
** CID 467059: Integer handling issues (INCOMPATIBLE_CAST)
*** CID 467059: Integer handling issues (INCOMPATIBLE_CAST)
/drivers/mtd/nvmxip/nvmxip_qspi.c: 47 in nvmxip_qspi_of_to_plat()
41 ret = dev_read_u32(dev, "lba_shift", >lba_shift);
42 if (ret) {
43 log_err("[%s]: can not get lba_shift from device
tree\n", dev->name);
44 return -EINVAL;
45 }
46
>>> CID 467059: Integer handling issues (INCOMPATIBLE_CAST)
>>> Pointer ">lba" points to an object whose effective type is
>>> "unsigned long" (64 bits, unsigned) but is dereferenced as a narrower
>>> "unsigned int" (32 bits, unsigned). This may lead to unexpected results
>>> depending on machine endianness.
47 ret = dev_read_u32(dev, "lba", (u32 *)>lba);
48 if (ret) {
49 log_err("[%s]: can not get lba from device tree\n", dev->name);
50 return -EINVAL;
51 }
52
** CID 467058: Insecure data handling (TAINTED_SCALAR)
*** CID 467058: Insecure data handling (TAINTED_SCALAR)
/drivers/core/ofnode.c: 1629 in ofnode_write_u32()
1623log_debug("%s = %x", propname, value);
1624val = malloc(sizeof(*val));
1625if (!val)
1626return -ENOMEM;
1627*val = cpu_to_fdt32(value);
1628
>>> CID 467058: Insecure data handling (TAINTED_SCALAR)
>>> Passing tainted expression "*val" to "ofnode_write_prop", which uses it
>>> as an offset.
1629return ofnode_write_prop(node, propname, val,
sizeof(value), true);
1630 }
1631
1632 int ofnode_write_u64(ofnode node, const char *propname, u64 value)
1633 {
1634fdt64_t *val;
** CID 467057: Uninitialized variables (UNINIT)
*** CID 467057: Uninitialized variables (UNINIT)
/boot/bootflow.c: 320 in iter_incr()
314 * Probe the bootdev. This does not
probe any attached
315 * block device, since they are siblings
316 */
317 ret = device_probe(dev);
318 log_debug("probe %s %d\n", dev->name, ret);
319 if (!log_msg_ret("probe", ret))
>>> CID 467057: Uninitialized variables (UNINIT)
>>> Using uninitialized value "method_flags" when calling
>>> "bootflow_iter_set_dev".
320 bootflow_iter_set_dev(iter,
dev, method_flags);
321 }
322 }
323
324 /* if there are no more bootdevs, give up */
325 if (ret)
** CID 467056: Control flow issues (NO_EFFECT)
/common/cli_readline.c: 321 in
Re: Fwd: New Defects reported by Coverity Scan for Das U-Boot
Hi Tom,
>
> *** CID 464361: Control flow issues (DEADCODE)
> /drivers/firmware/arm-ffa/arm-ffa-uclass.c: 148 in ffa_print_error_log()
> 142
> 143 if (ffa_id < FFA_FIRST_ID || ffa_id > FFA_LAST_ID)
> 144 return -EINVAL;
> 145
> 146 abi_idx = FFA_ID_TO_ERRMAP_ID(ffa_id);
> 147 if (abi_idx < 0 || abi_idx >= FFA_ERRMAP_COUNT)
> >>> CID 464361: Control flow issues (DEADCODE)
> >>> Execution cannot reach this statement: "return -22;".
> 148 return -EINVAL;
This is a false positive.
abi_idx value could end up matching this condition "(abi_idx < 0 || abi_idx >=
FFA_ERRMAP_COUNT)".
This happens when ffa_id value is above the allowed bounds. Example: when
ffa_id is 0x50 or 0x80
ffa_print_error_log(0x50, ...); /* exceeding lower bound */
ffa_print_error_log(0x80, ...); /* exceeding upper bound */
In these cases "return -EINVAL;" is executed.
> ...
>
> *** CID 464360: Control flow issues (NO_EFFECT)
> /drivers/firmware/arm-ffa/arm-ffa-uclass.c: 207 in ffa_get_version_hdlr()
> 201 major = GET_FFA_MAJOR_VERSION(res.a0);
> 202 minor = GET_FFA_MINOR_VERSION(res.a0);
> 203
> 204 log_debug("FF-A driver %d.%d\nFF-A framework %d.%d\n",
> 205 FFA_MAJOR_VERSION, FFA_MINOR_VERSION, major, minor);
> 206
> >>> CID 464360: Control flow issues (NO_EFFECT)
> >>> This greater-than-or-equal-to-zero comparison of an unsigned value is
> >>> always true. "minor >= 0".
> 207 if (major == FFA_MAJOR_VERSION && minor >= FFA_MINOR_VERSION)
> {
Providing the facts that:
#define FFA_MINOR_VERSION (0)
u16 minor;
Yes, currently this condition is always true: minor >= FFA_MINOR_VERSION
However, we might upgrade FFA_MINOR_VERSION in the future. If we remove the
"minor >= FFA_MINOR_VERSION" ,
non compatible versions could pass which we don't want.
To keep this code scalable, I think it's better to keep this condition.
> ...
>
> *** CID 464359:(PASS_BY_VALUE)
> /drivers/firmware/arm-ffa/arm-ffa-uclass.c: 168 in invoke_ffa_fn()
> 162 * @args: FF-A ABI arguments to be copied to Xn registers
> 163 * @res: FF-A ABI return data to be copied from Xn registers
> 164 *
> 165 * Calls low level SMC implementation.
> 166 * This function should be implemented by the user driver.
> 167 */
> >>> CID 464359:(PASS_BY_VALUE)
> >>> Passing parameter args of type "ffa_value_t" (size 144 bytes) by
> >>> value, which exceeds the low threshold of 128 bytes.
> 168 void __weak invoke_ffa_fn(ffa_value_t args, ffa_value_t *res)
We are using invoke_ffa_fn with the same arguments as in linux. The aim is to
use the same interfaces as in the Linux FF-A
driver to make porting code easier.
In Linux, args is passed by value [1].
ffa_value_t is a structure with 18 "unsigned long" fields. So, the size is
fixed.
[1]: invoke_ffa_fn arguments in the Linux FF-A driver
https://elixir.bootlin.com/linux/v6.6-rc6/source/drivers/firmware/arm_ffa/driver.c#L115
https://elixir.bootlin.com/linux/v6.6-rc6/source/drivers/firmware/arm_ffa/driver.c#L54
https://elixir.bootlin.com/linux/v6.6-rc6/source/drivers/firmware/arm_ffa/common.h#L15
[2]: include/linux/arm-smccc.h
> 169 {
> 170 }
> 171
> 172 /**
> 173 * ffa_get_version_hdlr() - FFA_VERSION handler function
> /drivers/firmware/arm-ffa/ffa-emul-uclass.c: 673 in invoke_ffa_fn()
> 667 * invoke_ffa_fn() - SMC wrapper
> 668 * @args: FF-A ABI arguments to be copied to Xn registers
> 669 * @res: FF-A ABI return data to be copied from Xn registers
> 670 *
> 671 * Calls the emulated SMC call.
> 672 */
> >>> CID 464359:(PASS_BY_VALUE)
> >>> Passing parameter args of type "ffa_value_t" (size 144 bytes) by
> >>> value, which exceeds the low threshold of 128 bytes.
> 673 void invoke_ffa_fn(ffa_value_t args, ffa_value_t *res)
Same feedback as above.
Cheers,
Abdellatif
Re: Fwd: New Defects reported by Coverity Scan for Das U-Boot
On Mon, Aug 28, 2023 at 01:09:17PM -0300, Alvaro Fernando García wrote: > Hello, > > El jue, 24 ago. 2023 06:27, Abdellatif El Khlifi < > abdellatif.elkhl...@arm.com> escribió: > > > Hi Tom, > > > > > Here's the latest report > > > > > > -- Forwarded message - > > > From: > > > Date: Mon, Aug 21, 2023 at 4:30 PM > > > Subject: New Defects reported by Coverity Scan for Das U-Boot > > > To: > > > > > > > > > Hi, > > > > > > Please find the latest report on new defect(s) introduced to Das > > > U-Boot found with Coverity Scan. > > > > > > 4 new defect(s) introduced to Das U-Boot found with Coverity Scan. > > > > > > > > > New defect(s) Reported-by: Coverity Scan > > > Showing 4 of 4 defect(s) > > > > > > > > > ** CID 464361: Control flow issues (DEADCODE) > > > /drivers/firmware/arm-ffa/arm-ffa-uclass.c: 148 in ffa_print_error_log() > > > > Well received, I started working on that. > > I'll provide a fix after coming back fom holidays (mid September) > > > > Cheers, > > Abdellatif > > > > Is there something I could do to help with this? Everyone is free to work on these issues, yes. -- Tom signature.asc Description: PGP signature
Re: Fwd: New Defects reported by Coverity Scan for Das U-Boot
Hello, El jue, 24 ago. 2023 06:27, Abdellatif El Khlifi < abdellatif.elkhl...@arm.com> escribió: > Hi Tom, > > > Here's the latest report > > > > -- Forwarded message - > > From: > > Date: Mon, Aug 21, 2023 at 4:30 PM > > Subject: New Defects reported by Coverity Scan for Das U-Boot > > To: > > > > > > Hi, > > > > Please find the latest report on new defect(s) introduced to Das > > U-Boot found with Coverity Scan. > > > > 4 new defect(s) introduced to Das U-Boot found with Coverity Scan. > > > > > > New defect(s) Reported-by: Coverity Scan > > Showing 4 of 4 defect(s) > > > > > > ** CID 464361: Control flow issues (DEADCODE) > > /drivers/firmware/arm-ffa/arm-ffa-uclass.c: 148 in ffa_print_error_log() > > Well received, I started working on that. > I'll provide a fix after coming back fom holidays (mid September) > > Cheers, > Abdellatif > Is there something I could do to help with this?
Re: Fwd: New Defects reported by Coverity Scan for Das U-Boot
Hi Tom, > Here's the latest report > > -- Forwarded message - > From: > Date: Mon, Aug 21, 2023 at 4:30 PM > Subject: New Defects reported by Coverity Scan for Das U-Boot > To: > > > Hi, > > Please find the latest report on new defect(s) introduced to Das > U-Boot found with Coverity Scan. > > 4 new defect(s) introduced to Das U-Boot found with Coverity Scan. > > > New defect(s) Reported-by: Coverity Scan > Showing 4 of 4 defect(s) > > > ** CID 464361: Control flow issues (DEADCODE) > /drivers/firmware/arm-ffa/arm-ffa-uclass.c: 148 in ffa_print_error_log() Well received, I started working on that. I'll provide a fix after coming back fom holidays (mid September) Cheers, Abdellatif
Fwd: New Defects reported by Coverity Scan for Das U-Boot
Here's the latest report
-- Forwarded message -
From:
Date: Mon, Aug 21, 2023 at 4:30 PM
Subject: New Defects reported by Coverity Scan for Das U-Boot
To:
Hi,
Please find the latest report on new defect(s) introduced to Das
U-Boot found with Coverity Scan.
4 new defect(s) introduced to Das U-Boot found with Coverity Scan.
New defect(s) Reported-by: Coverity Scan
Showing 4 of 4 defect(s)
** CID 464362: Integer handling issues (OVERFLOW_BEFORE_WIDEN)
/drivers/video/pwm_backlight.c: 68 in set_pwm()
*** CID 464362: Integer handling issues (OVERFLOW_BEFORE_WIDEN)
/drivers/video/pwm_backlight.c: 68 in set_pwm()
62 {
63 u64 width;
64 uint duty_cycle;
65 int ret;
66
67 if (priv->period_ns) {
>>> CID 464362: Integer handling issues (OVERFLOW_BEFORE_WIDEN)
>>> Potentially overflowing expression "priv->period_ns * (priv->cur_level
>>> - priv->min_level)" with type "unsigned int" (32 bits, unsigned) is
>>> evaluated using 32-bit arithmetic, and then used in a context that expects
>>> an expression of type "u64" (64 bits, unsigned).
68 width = priv->period_ns * (priv->cur_level - priv->min_level);
69 duty_cycle = div_u64(width,
70 (priv->max_level - priv->min_level));
71 ret = pwm_set_config(priv->pwm, priv->channel, priv->period_ns,
72 duty_cycle);
73 } else {
** CID 464361: Control flow issues (DEADCODE)
/drivers/firmware/arm-ffa/arm-ffa-uclass.c: 148 in ffa_print_error_log()
*** CID 464361: Control flow issues (DEADCODE)
/drivers/firmware/arm-ffa/arm-ffa-uclass.c: 148 in ffa_print_error_log()
142
143 if (ffa_id < FFA_FIRST_ID || ffa_id > FFA_LAST_ID)
144 return -EINVAL;
145
146 abi_idx = FFA_ID_TO_ERRMAP_ID(ffa_id);
147 if (abi_idx < 0 || abi_idx >= FFA_ERRMAP_COUNT)
>>> CID 464361: Control flow issues (DEADCODE)
>>> Execution cannot reach this statement: "return -22;".
148 return -EINVAL;
149
150 if (!err_msg_map[abi_idx].err_str[err_idx])
151 return -EINVAL;
152
153 log_err("%s\n", err_msg_map[abi_idx].err_str[err_idx]);
** CID 464360: Control flow issues (NO_EFFECT)
/drivers/firmware/arm-ffa/arm-ffa-uclass.c: 207 in ffa_get_version_hdlr()
*** CID 464360: Control flow issues (NO_EFFECT)
/drivers/firmware/arm-ffa/arm-ffa-uclass.c: 207 in ffa_get_version_hdlr()
201 major = GET_FFA_MAJOR_VERSION(res.a0);
202 minor = GET_FFA_MINOR_VERSION(res.a0);
203
204 log_debug("FF-A driver %d.%d\nFF-A framework %d.%d\n",
205 FFA_MAJOR_VERSION, FFA_MINOR_VERSION, major, minor);
206
>>> CID 464360: Control flow issues (NO_EFFECT)
>>> This greater-than-or-equal-to-zero comparison of an unsigned value is
>>> always true. "minor >= 0".
207 if (major == FFA_MAJOR_VERSION && minor >= FFA_MINOR_VERSION) {
208 log_debug("FF-A versions are compatible\n");
209
210 if (dev) {
211 uc_priv = dev_get_uclass_priv(dev);
212 if (uc_priv)
** CID 464359:(PASS_BY_VALUE)
/drivers/firmware/arm-ffa/arm-ffa-uclass.c: 168 in invoke_ffa_fn()
/drivers/firmware/arm-ffa/ffa-emul-uclass.c: 673 in invoke_ffa_fn()
*** CID 464359:(PASS_BY_VALUE)
/drivers/firmware/arm-ffa/arm-ffa-uclass.c: 168 in invoke_ffa_fn()
162 * @args: FF-A ABI arguments to be copied to Xn registers
163 * @res: FF-A ABI return data to be copied from Xn registers
164 *
165 * Calls low level SMC implementation.
166 * This function should be implemented by the user driver.
167 */
>>> CID 464359:(PASS_BY_VALUE)
>>> Passing parameter args of type "ffa_value_t" (size 144 bytes) by value,
>>> which exceeds the low threshold of 128 bytes.
168 void __weak invoke_ffa_fn(ffa_value_t args, ffa_value_t *res)
169 {
170 }
171
172 /**
173 * ffa_get_version_hdlr() - FFA_VERSION handler function
/drivers/firmware/arm-ffa/ffa-emul-uclass.c: 673 in invoke_ffa_fn()
667 * invoke_ffa_fn() - SMC wrapper
668 * @args: FF-A ABI arguments to be copied to Xn registers
669 * @res: FF-A ABI return data to be copied from Xn registers
670 *
671 * Calls the emulated SMC call.
672 */
>>> CID 464359:(PASS_BY_VALUE)
>>> Passing parameter args of type "ffa_value_t" (size 144 bytes) by value,
>>> which
[tom.r...@gmail.com: Fwd: New Defects reported by Coverity Scan for Das U-Boot]
Here's the latest report.
-- Forwarded message -
From:
Date: Tue, Jul 25, 2023 at 5:29 PM
Subject: New Defects reported by Coverity Scan for Das U-Boot
To:
Hi,
Please find the latest report on new defect(s) introduced to Das
U-Boot found with Coverity Scan.
5 new defect(s) introduced to Das U-Boot found with Coverity Scan.
30 defect(s), reported by Coverity Scan earlier, were marked fixed in
the recent build analyzed by Coverity Scan.
New defect(s) Reported-by: Coverity Scan
Showing 5 of 5 defect(s)
** CID 463147: Insecure data handling (TAINTED_SCALAR)
*** CID 463147: Insecure data handling (TAINTED_SCALAR)
/common/fdt_support.c: 1115 in fdt_copy_fixed_partitions()
1109res = fdt_setprop_string(blob, suboff, "label",
1110
ofnode_read_string(subnode, "label"));
if (res)
1112return res;
1113
1114reg = ofnode_get_property(subnode, "reg", );
>>> CID 463147: Insecure data handling (TAINTED_SCALAR)
>>> Passing tainted expression "len" to "fdt_setprop", which uses it as an
>>> offset.
1115res = fdt_setprop(blob, suboff, "reg",
reg, len);
1116if (res)
1117return res;
1118}
1119
1120/* go to next fixed-partitions node */
** CID 463146: Null pointer dereferences (NULL_RETURNS)
*** CID 463146: Null pointer dereferences (NULL_RETURNS)
/common/fdt_support.c: 1115 in fdt_copy_fixed_partitions()
1109res = fdt_setprop_string(blob, suboff, "label",
1110
ofnode_read_string(subnode, "label"));
if (res)
1112return res;
1113
1114reg = ofnode_get_property(subnode, "reg", );
>>> CID 463146: Null pointer dereferences (NULL_RETURNS)
>>> Dereferencing a pointer that might be "NULL" "reg" when calling
>>> "fdt_setprop".
1115res = fdt_setprop(blob, suboff, "reg",
reg, len);
1116if (res)
1117return res;
1118}
1119
1120/* go to next fixed-partitions node */
** CID 463145: Error handling issues (CHECKED_RETURN)
/lib/efi_loader/efi_firmware.c: 176 in efi_firmware_get_lsv_from_dtb()
*** CID 463145: Error handling issues (CHECKED_RETURN)
/lib/efi_loader/efi_firmware.c: 176 in efi_firmware_get_lsv_from_dtb()
170 fdt_for_each_subnode(offset, fdt, parent) {
171 efi_guid_t guid;
172
173 guid_str = fdt_getprop(fdt, offset,
"image-type-id", );
174 if (!guid_str)
175 continue;
>>> CID 463145: Error handling issues (CHECKED_RETURN)
>>> Calling "uuid_str_to_bin" without checking return value (as is done
>>> elsewhere 7 out of 8 times).
176 uuid_str_to_bin(guid_str, guid.b, UUID_STR_FORMAT_GUID);
177
178 val = fdt_getprop(fdt, offset, "image-index", );
179 if (!val)
180 continue;
181 index = fdt32_to_cpu(*val);
** CID 463144:(CHECKED_RETURN)
/boot/expo.c: 257 in expo_apply_theme()
/boot/expo.c: 258 in expo_apply_theme()
/boot/expo.c: 259 in expo_apply_theme()
*** CID 463144:(CHECKED_RETURN)
/boot/expo.c: 257 in expo_apply_theme()
251 struct expo_theme *theme = >theme;
252 int ret;
253
254 log_debug("Applying theme %s\n", ofnode_get_name(node));
255
256 memset(theme, '\0', sizeof(struct expo_theme));
>>> CID 463144:(CHECKED_RETURN)
>>> Calling "ofnode_read_u32" without checking return value (as is done
>>> elsewhere 33 out of 41 times).
257 ofnode_read_u32(node, "font-size", >font_size);
258 ofnode_read_u32(node, "menu-inset", >menu_inset);
259 ofnode_read_u32(node, "menuitem-gap-y", >menuitem_gap_y);
260
261 list_for_each_entry(scn, >scene_head, sibling) {
262 ret = scene_apply_theme(scn, theme);
/boot/expo.c: 258 in expo_apply_theme()
252 int ret;
253
254 log_debug("Applying theme %s\n", ofnode_get_name(node));
255
256 memset(theme, '\0', sizeof(struct expo_theme));
257 ofnode_read_u32(node, "font-size", >font_size);
>>> CID 463144:(CHECKED_RETURN)
>>>
[tom.r...@gmail.com: Fwd: New Defects reported by Coverity Scan for Das U-Boot]
Here's the latest report.
-- Forwarded message -
From:
Date: Mon, May 29, 2023, 11:10 AM
Subject: New Defects reported by Coverity Scan for Das U-Boot
To:
Hi,
Please find the latest report on new defect(s) introduced to Das U-Boot
found with Coverity Scan.
2 new defect(s) introduced to Das U-Boot found with Coverity Scan.
New defect(s) Reported-by: Coverity Scan
Showing 2 of 2 defect(s)
** CID 461871: Null pointer dereferences (NULL_RETURNS)
/tools/renesas_spkgimage.c: 56 in spkgimage_parse_config_line()
*** CID 461871: Null pointer dereferences (NULL_RETURNS)
/tools/renesas_spkgimage.c: 56 in spkgimage_parse_config_line()
50 char *saveptr;
51 char *delim = "\t ";
52 char *name = strtok_r(line, delim, );
53 char *val_str = strtok_r(NULL, delim, );
54 int value = atoi(val_str);
55
>>> CID 461871: Null pointer dereferences (NULL_RETURNS)
>>> Dereferencing a pointer that might be "NULL" "name" when calling
"strcmp". [Note: The source code implementation of the function has been
overridden by a builtin model.]
56 if (!strcmp("VERSION", name)) {
57 conf.version = check_range(name, value, 1, 15);
58 } else if (!strcmp("NAND_ECC_ENABLE", name)) {
59 conf.ecc_enable = check_range(name, value, 0, 1);
60 } else if (!strcmp("NAND_ECC_BLOCK_SIZE", name)) {
61 conf.ecc_block_size = check_range(name, value, 0, 2);
** CID 461870: Resource leaks (RESOURCE_LEAK)
/tools/renesas_spkgimage.c: 106 in spkgimage_parse_config_file()
*** CID 461870: Resource leaks (RESOURCE_LEAK)
/tools/renesas_spkgimage.c: 106 in spkgimage_parse_config_file()
100
101 /* Strip any trailing newline */
102 line[strcspn(line, "\n")] = 0;
103
104 /* Parse the line */
105 if (spkgimage_parse_config_line(line, line_num))
>>> CID 461870: Resource leaks (RESOURCE_LEAK)
>>> Variable "fcfg" going out of scope leaks the storage it points to.
106 return -EINVAL;
107 }
108
109 fclose(fcfg);
110
111 /* Avoid divide-by-zero later on */
--
Tom
signature.asc
Description: PGP signature
Re: Fwd: New Defects reported by Coverity Scan for Das U-Boot
On 2023-05-08 1:20 p.m., Tom Rini wrote:
Here's the latest defect report:
-- Forwarded message -
From:
Date: Mon, May 8, 2023, 2:29 PM
Subject: New Defects reported by Coverity Scan for Das U-Boot
To:
Hi,
Please find the latest report on new defect(s) introduced to Das U-Boot
found with Coverity Scan.
5 new defect(s) introduced to Das U-Boot found with Coverity Scan.
1 defect(s), reported by Coverity Scan earlier, were marked fixed in the
recent build analyzed by Coverity Scan.
New defect(s) Reported-by: Coverity Scan
Showing 5 of 5 defect(s)
** CID 453851: Memory - corruptions (OVERLAPPING_COPY)
/cmd/net.c: 279 in netboot_update_env()
*** CID 453851: Memory - corruptions (OVERLAPPING_COPY)
/cmd/net.c: 279 in netboot_update_env()
273
274 if (IS_ENABLED(CONFIG_IPV6)) {
275 if (!ip6_is_unspecified_addr(_ip6) ||
276 net_prefix_length != 0) {
277 sprintf(tmp, "%pI6c", _ip6);
278 if (net_prefix_length != 0)
CID 453851: Memory - corruptions (OVERLAPPING_COPY)
In the call to function "sprintf", the arguments "tmp" and "tmp"
Just submitted a patch to fix 453851.
may point to the same object.
279 sprintf(tmp, "%s/%d", tmp,
net_prefix_length);
280
281 env_set("ip6addr", tmp);
282 }
283
284 if (!ip6_is_unspecified_addr(_server_ip6)) {
** CID 450971: Insecure data handling (TAINTED_SCALAR)
/net/ndisc.c: 391 in process_ra()
*** CID 450971: Insecure data handling (TAINTED_SCALAR)
/net/ndisc.c: 391 in process_ra()
385 /* Ignore the packet if router lifetime is 0. */
386 if (!icmp->icmp6_rt_lifetime)
387 return -EOPNOTSUPP;
388
389 /* Processing the options */
390 option = msg->opt;
CID 450971: Insecure data handling (TAINTED_SCALAR)
Using tainted variable "remaining_option_len" as a loop boundary.
391 while (remaining_option_len > 0) {
392 /* The 2nd byte of the option is its length. */
393 option_len = option[1];
394 /* All included options should have a positive
length. */
395 if (option_len == 0)
396 return -EINVAL;
** CID 450969: Security best practices violations (DC.WEAK_CRYPTO)
/net/ndisc.c: 209 in ip6_send_rs()
*** CID 450969: Security best practices violations (DC.WEAK_CRYPTO)
/net/ndisc.c: 209 in ip6_send_rs()
203icmp_len, PROT_ICMPV6, pcsum);
204 msg->icmph.icmp6_cksum = csum;
205 pkt += icmp_len;
206
207 /* Wait up to 1 second if it is the first try to get the RA
*/
208 if (retry_count == 0)
CID 450969: Security best practices violations (DC.WEAK_CRYPTO)
"rand" should not be used for security-related applications,
because linear congruential algorithms are too easy to break.
209 udelay(((unsigned int)rand() % 100) *
MAX_SOLICITATION_DELAY);
210
211 /* send it! */
212 net_send_packet(net_tx_packet, (pkt - net_tx_packet));
213
214 retry_count++;
** CID 436282:(DC.WEAK_CRYPTO)
/net/dhcpv6.c: 621 in dhcp6_state_machine()
/net/dhcpv6.c: 627 in dhcp6_state_machine()
/net/dhcpv6.c: 628 in dhcp6_state_machine()
/net/dhcpv6.c: 662 in dhcp6_state_machine()
/net/dhcpv6.c: 613 in dhcp6_state_machine()
*** CID 436282:(DC.WEAK_CRYPTO)
/net/dhcpv6.c: 621 in dhcp6_state_machine()
615 /* handle state machine entry conditions */
616 if (sm_params.curr_state != sm_params.next_state) {
617 sm_params.retry_cnt = 0;
618
619 if (sm_params.next_state == DHCP6_SOLICIT) {
620 /* delay a random ammount (special for
SOLICIT) */
CID 436282:(DC.WEAK_CRYPTO)
"rand" should not be used for security-related applications,
because linear congruential algorithms are too easy to break.
621 udelay((rand() % SOL_MAX_DELAY_MS) * 1000);
622 /* init timestamp variables after SOLICIT
delay */
623 sm_params.dhcp6_start_ms = get_timer(0);
624 sm_params.dhcp6_retry_start_ms =
sm_params.dhcp6_start_ms;
625 sm_params.dhcp6_retry_ms =
sm_params.dhcp6_start_ms;
Re: Fwd: New Defects reported by Coverity Scan for Das U-Boot
On 5/8/2023 3:20 PM, Tom Rini wrote:
Here's the latest defect report:
-- Forwarded message -
From:
Date: Mon, May 8, 2023, 2:29 PM
Subject: New Defects reported by Coverity Scan for Das U-Boot
To:
Hi,
Please find the latest report on new defect(s) introduced to Das U-Boot
found with Coverity Scan.
5 new defect(s) introduced to Das U-Boot found with Coverity Scan.
1 defect(s), reported by Coverity Scan earlier, were marked fixed in the
recent build analyzed by Coverity Scan.
New defect(s) Reported-by: Coverity Scan
Showing 5 of 5 defect(s)
** CID 450971: Insecure data handling (TAINTED_SCALAR)
/net/ndisc.c: 391 in process_ra()
*** CID 450971: Insecure data handling (TAINTED_SCALAR)
/net/ndisc.c: 391 in process_ra()
385 /* Ignore the packet if router lifetime is 0. */
386 if (!icmp->icmp6_rt_lifetime)
387 return -EOPNOTSUPP;
388
389 /* Processing the options */
390 option = msg->opt;
CID 450971: Insecure data handling (TAINTED_SCALAR)
Using tainted variable "remaining_option_len" as a loop boundary.
391 while (remaining_option_len > 0) {
392 /* The 2nd byte of the option is its length. */
393 option_len = option[1];
394 /* All included options should have a positive
length. */
395 if (option_len == 0)
396 return -EINVAL;
The problem here is that although the lower bound of the variable
remaining_option_len is checked, the upper bound is not checked.
Coverity is complaining that the function's argument len which is read
from a packet content is assigned to remaining_option_len and therefore
has made it a tainted scalar.
I will compare the value of len with ETH_MAX_MTU constant and make sure
it is less than that as shown below.
if(len > ETH_MAX_MTU) return-EMSGSIZE;
** CID 450969: Security best practices violations (DC.WEAK_CRYPTO)
/net/ndisc.c: 209 in ip6_send_rs()
*** CID 450969: Security best practices violations (DC.WEAK_CRYPTO)
/net/ndisc.c: 209 in ip6_send_rs()
203icmp_len, PROT_ICMPV6, pcsum);
204 msg->icmph.icmp6_cksum = csum;
205 pkt += icmp_len;
206
207 /* Wait up to 1 second if it is the first try to get the RA
*/
208 if (retry_count == 0)
CID 450969: Security best practices violations (DC.WEAK_CRYPTO)
"rand" should not be used for security-related applications,
because linear congruential algorithms are too easy to break.
209 udelay(((unsigned int)rand() % 100) *
MAX_SOLICITATION_DELAY);
210
211 /* send it! */
212 net_send_packet(net_tx_packet, (pkt - net_tx_packet));
213
214 retry_count++;
This is a false positive. The function rand() is not used for encryption
here. It is used to just make a random delay to avoid collisions on the
network. It has nothing to do with encryption.
Fwd: New Defects reported by Coverity Scan for Das U-Boot
Here's the latest defect report:
-- Forwarded message -
From:
Date: Mon, May 8, 2023, 2:29 PM
Subject: New Defects reported by Coverity Scan for Das U-Boot
To:
Hi,
Please find the latest report on new defect(s) introduced to Das U-Boot
found with Coverity Scan.
5 new defect(s) introduced to Das U-Boot found with Coverity Scan.
1 defect(s), reported by Coverity Scan earlier, were marked fixed in the
recent build analyzed by Coverity Scan.
New defect(s) Reported-by: Coverity Scan
Showing 5 of 5 defect(s)
** CID 453851: Memory - corruptions (OVERLAPPING_COPY)
/cmd/net.c: 279 in netboot_update_env()
*** CID 453851: Memory - corruptions (OVERLAPPING_COPY)
/cmd/net.c: 279 in netboot_update_env()
273
274 if (IS_ENABLED(CONFIG_IPV6)) {
275 if (!ip6_is_unspecified_addr(_ip6) ||
276 net_prefix_length != 0) {
277 sprintf(tmp, "%pI6c", _ip6);
278 if (net_prefix_length != 0)
>>> CID 453851: Memory - corruptions (OVERLAPPING_COPY)
>>> In the call to function "sprintf", the arguments "tmp" and "tmp"
may point to the same object.
279 sprintf(tmp, "%s/%d", tmp,
net_prefix_length);
280
281 env_set("ip6addr", tmp);
282 }
283
284 if (!ip6_is_unspecified_addr(_server_ip6)) {
** CID 450971: Insecure data handling (TAINTED_SCALAR)
/net/ndisc.c: 391 in process_ra()
*** CID 450971: Insecure data handling (TAINTED_SCALAR)
/net/ndisc.c: 391 in process_ra()
385 /* Ignore the packet if router lifetime is 0. */
386 if (!icmp->icmp6_rt_lifetime)
387 return -EOPNOTSUPP;
388
389 /* Processing the options */
390 option = msg->opt;
>>> CID 450971: Insecure data handling (TAINTED_SCALAR)
>>> Using tainted variable "remaining_option_len" as a loop boundary.
391 while (remaining_option_len > 0) {
392 /* The 2nd byte of the option is its length. */
393 option_len = option[1];
394 /* All included options should have a positive
length. */
395 if (option_len == 0)
396 return -EINVAL;
** CID 450969: Security best practices violations (DC.WEAK_CRYPTO)
/net/ndisc.c: 209 in ip6_send_rs()
*** CID 450969: Security best practices violations (DC.WEAK_CRYPTO)
/net/ndisc.c: 209 in ip6_send_rs()
203icmp_len, PROT_ICMPV6, pcsum);
204 msg->icmph.icmp6_cksum = csum;
205 pkt += icmp_len;
206
207 /* Wait up to 1 second if it is the first try to get the RA
*/
208 if (retry_count == 0)
>>> CID 450969: Security best practices violations (DC.WEAK_CRYPTO)
>>> "rand" should not be used for security-related applications,
because linear congruential algorithms are too easy to break.
209 udelay(((unsigned int)rand() % 100) *
MAX_SOLICITATION_DELAY);
210
211 /* send it! */
212 net_send_packet(net_tx_packet, (pkt - net_tx_packet));
213
214 retry_count++;
** CID 436282:(DC.WEAK_CRYPTO)
/net/dhcpv6.c: 621 in dhcp6_state_machine()
/net/dhcpv6.c: 627 in dhcp6_state_machine()
/net/dhcpv6.c: 628 in dhcp6_state_machine()
/net/dhcpv6.c: 662 in dhcp6_state_machine()
/net/dhcpv6.c: 613 in dhcp6_state_machine()
*** CID 436282:(DC.WEAK_CRYPTO)
/net/dhcpv6.c: 621 in dhcp6_state_machine()
615 /* handle state machine entry conditions */
616 if (sm_params.curr_state != sm_params.next_state) {
617 sm_params.retry_cnt = 0;
618
619 if (sm_params.next_state == DHCP6_SOLICIT) {
620 /* delay a random ammount (special for
SOLICIT) */
>>> CID 436282:(DC.WEAK_CRYPTO)
>>> "rand" should not be used for security-related applications,
because linear congruential algorithms are too easy to break.
621 udelay((rand() % SOL_MAX_DELAY_MS) * 1000);
622 /* init timestamp variables after SOLICIT
delay */
623 sm_params.dhcp6_start_ms = get_timer(0);
624 sm_params.dhcp6_retry_start_ms =
sm_params.dhcp6_start_ms;
625 sm_params.dhcp6_retry_ms =
sm_params.dhcp6_start_ms;
626 /* init transaction and ia_id */
[tom.r...@gmail.com: Fwd: New Defects reported by Coverity Scan for Das U-Boot]
Here's the latest report.
-- Forwarded message -
From:
Date: Mon, Mar 27, 2023 at 2:36 PM
Subject: New Defects reported by Coverity Scan for Das U-Boot
To:
Hi,
Please find the latest report on new defect(s) introduced to Das
U-Boot found with Coverity Scan.
6 new defect(s) introduced to Das U-Boot found with Coverity Scan.
2 defect(s), reported by Coverity Scan earlier, were marked fixed in
the recent build analyzed by Coverity Scan.
New defect(s) Reported-by: Coverity Scan
Showing 6 of 6 defect(s)
** CID 451089: Incorrect expression (EVALUATION_ORDER)
/lib/efi_loader/efi_device_path.c: 752 in dp_fill()
*** CID 451089: Incorrect expression (EVALUATION_ORDER)
/lib/efi_loader/efi_device_path.c: 752 in dp_fill()
746 memcpy(>ns_id, _id, sizeof(ns_id));
747 return [1];
748 }
749 #endif
750 #if defined(CONFIG_USB)
751 case UCLASS_MASS_STORAGE: {
>>> CID 451089: Incorrect expression (EVALUATION_ORDER)
>>> In "desc = desc = dev_get_uclass_plat(dev)", "desc" is written twice
>>> with the same value.
752 struct blk_desc *desc = desc =
dev_get_uclass_plat(dev);
753 struct efi_device_path_controller *dp =
754 dp_fill(buf, dev->parent);
755
756 dp->dp.type =
DEVICE_PATH_TYPE_HARDWARE_DEVICE;
757 dp->dp.sub_type =
DEVICE_PATH_SUB_TYPE_CONTROLLER;
** CID 450973:(TAINTED_SCALAR)
*** CID 450973:(TAINTED_SCALAR)
/test/cmd/fdt.c: 133 in make_fuller_fdt()
127 ut_assertok(fdt_property_cell(fdt, "#size-cells", 0));
128 ut_assertok(fdt_property_string(fdt, "compatible",
"u-boot,fdt-subnode-test-device"));
129 ut_assertok(fdt_end_node(fdt));
130 ut_assertok(fdt_end_node(fdt));
131
132 ut_assertok(fdt_end_node(fdt));
>>> CID 450973:(TAINTED_SCALAR)
>>> Passing tainted expression "fdt->size_dt_strings" to "fdt_finish",
>>> which uses it as an offset.
133 ut_assertok(fdt_finish(fdt));
134
135 return 0;
136 }
137
138 /* Test 'fdt addr' getting/setting address */
/test/cmd/fdt.c: 133 in make_fuller_fdt()
127 ut_assertok(fdt_property_cell(fdt, "#size-cells", 0));
128 ut_assertok(fdt_property_string(fdt, "compatible",
"u-boot,fdt-subnode-test-device"));
129 ut_assertok(fdt_end_node(fdt));
130 ut_assertok(fdt_end_node(fdt));
131
132 ut_assertok(fdt_end_node(fdt));
>>> CID 450973:(TAINTED_SCALAR)
>>> Passing tainted expression "fdt->size_dt_strings" to "fdt_finish",
>>> which uses it as an offset.
133 ut_assertok(fdt_finish(fdt));
134
135 return 0;
136 }
137
138 /* Test 'fdt addr' getting/setting address */
/test/cmd/fdt.c: 133 in make_fuller_fdt()
127 ut_assertok(fdt_property_cell(fdt, "#size-cells", 0));
128 ut_assertok(fdt_property_string(fdt, "compatible",
"u-boot,fdt-subnode-test-device"));
129 ut_assertok(fdt_end_node(fdt));
130 ut_assertok(fdt_end_node(fdt));
131
132 ut_assertok(fdt_end_node(fdt));
>>> CID 450973:(TAINTED_SCALAR)
>>> Passing tainted expression "fdt->size_dt_strings" to "fdt_finish",
>>> which uses it as an offset.
133 ut_assertok(fdt_finish(fdt));
134
135 return 0;
136 }
137
138 /* Test 'fdt addr' getting/setting address */
/test/cmd/fdt.c: 133 in make_fuller_fdt()
127 ut_assertok(fdt_property_cell(fdt, "#size-cells", 0));
128 ut_assertok(fdt_property_string(fdt, "compatible",
"u-boot,fdt-subnode-test-device"));
129 ut_assertok(fdt_end_node(fdt));
130 ut_assertok(fdt_end_node(fdt));
131
132 ut_assertok(fdt_end_node(fdt));
>>> CID 450973:(TAINTED_SCALAR)
>>> Passing tainted expression "fdt->size_dt_strings" to "fdt_finish",
>>> which uses it as an offset.
133 ut_assertok(fdt_finish(fdt));
134
135 return 0;
136 }
137
138 /* Test 'fdt addr' getting/setting address */
/test/cmd/fdt.c: 133 in make_fuller_fdt()
127 ut_assertok(fdt_property_cell(fdt, "#size-cells", 0));
128 ut_assertok(fdt_property_string(fdt, "compatible",
"u-boot,fdt-subnode-test-device"));
129 ut_assertok(fdt_end_node(fdt));
130 ut_assertok(fdt_end_node(fdt));
131
132 ut_assertok(fdt_end_node(fdt));
>>> CID 450973:(TAINTED_SCALAR)
>>> Passing tainted expression "fdt->size_dt_strings" to "fdt_finish",
>>> which uses it as an offset.
133
Fwd: New Defects reported by Coverity Scan for Das U-Boot
-- Forwarded message -
From:
Date: Mon, Feb 13, 2023, 6:50 PM
Subject: New Defects reported by Coverity Scan for Das U-Boot
To:
Hi,
Please find the latest report on new defect(s) introduced to Das U-Boot
found with Coverity Scan.
2 new defect(s) introduced to Das U-Boot found with Coverity Scan.
3 defect(s), reported by Coverity Scan earlier, were marked fixed in the
recent build analyzed by Coverity Scan.
New defect(s) Reported-by: Coverity Scan
Showing 2 of 2 defect(s)
** CID 436073: Resource leaks (RESOURCE_LEAK)
/tools/proftool.c: 1853 in make_flamegraph()
*** CID 436073: Resource leaks (RESOURCE_LEAK)
/tools/proftool.c: 1853 in make_flamegraph()
1847
1848if (make_flame_tree(out_format, ))
1849return -1;
1850
1851*str = '\0';
1852if (output_tree(fout, out_format, tree, str, sizeof(str),
0))
>>> CID 436073: Resource leaks (RESOURCE_LEAK)
>>> Variable "tree" going out of scope leaks the storage it points to.
1853return -1;
1854
1855return 0;
1856 }
1857
1858 /**
** CID 436072: Insecure data handling (TAINTED_SCALAR)
*** CID 436072: Insecure data handling (TAINTED_SCALAR)
/tools/proftool.c: 515 in read_trace()
509 switch (hdr.type) {
510 case TRACE_CHUNK_FUNCS:
511 /* Ignored at present */
512 break;
513
514 case TRACE_CHUNK_CALLS:
>>> CID 436072: Insecure data handling (TAINTED_SCALAR)
>>> Passing tainted expression "hdr.rec_count" to "read_calls", which
uses it as an allocation size.
515 if (read_calls(fin, hdr.rec_count))
516 return 1;
517 break;
518 }
519 }
520 return 0;
--
Tom
signature.asc
Description: PGP signature
[tom.r...@gmail.com: Fwd: New Defects reported by Coverity Scan for Das U-Boot]
- Forwarded message from Tom Rini -
Date: Tue, 31 Jan 2023 07:30:23 -0500
From: Tom Rini
To: tr...@konsulko.com
Subject: Fwd: New Defects reported by Coverity Scan for Das U-Boot
On Mon, Jan 30, 2023, 4:15 PM wrote:
> Hi,
>
> Please find the latest report on new defect(s) introduced to Das U-Boot
> found with Coverity Scan.
>
> 18 new defect(s) introduced to Das U-Boot found with Coverity Scan.
> 9 defect(s), reported by Coverity Scan earlier, were marked fixed in the
> recent build analyzed by Coverity Scan.
>
> New defect(s) Reported-by: Coverity Scan
> Showing 18 of 18 defect(s)
>
>
> ** CID 435669: Control flow issues (MISSING_BREAK)
> /lib/vsprintf.c: 681 in vsnprintf_internal()
>
>
>
>
> *** CID 435669: Control flow issues (MISSING_BREAK)
> /lib/vsprintf.c: 681 in vsnprintf_internal()
> 675 case 'x':
> 676 flags |= SMALL;
> 677 case 'X':
> 678 base = 16;
> 679 break;
> 680
> >>> CID 435669: Control flow issues (MISSING_BREAK)
> >>> The case for value "'d'" is not terminated by a "break" statement.
> 681 case 'd':
> 682 if (fmt[1] == 'E')
> 683 flags |= ERRSTR;
> 684 case 'i':
> 685 flags |= SIGN;
> 686 case 'u':
>
> ** CID 435668: Insecure data handling (TAINTED_SCALAR)
> /boot/image-fdt.c: 397 in select_fdt()
>
>
>
>
> *** CID 435668: Insecure data handling (TAINTED_SCALAR)
> /boot/image-fdt.c: 397 in select_fdt()
> 391 return -EFAULT;
> 392 }
> 393
> 394 debug(" Loading FDT from 0x%08lx to
> 0x%08lx\n",
> 395 image_data, load);
> 396
> >>> CID 435668: Insecure data handling (TAINTED_SCALAR)
> >>> Passing tainted expression "image_get_data_size(fdt_hdr)" to
> "memmove", which uses it as an offset. [Note: The source code
> implementation of the function has been overridden by a builtin model.]
> 397 memmove((void *)load,
> 398 (void *)image_data,
> 399 image_get_data_size(fdt_hdr));
> 400
> 401 fdt_addr = load;
> 402 break;
>
> ** CID 435667: Memory - corruptions (OVERRUN)
>
>
>
>
> *** CID 435667: Memory - corruptions (OVERRUN)
> /lib/zstd/decompress/zstd_decompress.c: 88 in ZSTD_DDictHashSet_getIndex()
> 82 #define DDICT_HASHSET_RESIZE_FACTOR 2
> 83
> 84 /* Hash function to determine starting position of dict insertion
> within the table
> 85 * Returns an index between [0, hashSet->ddictPtrTableSize]
> 86 */
> 87 static size_t ZSTD_DDictHashSet_getIndex(const ZSTD_DDictHashSet*
> hashSet, U32 dictID) {
> >>> CID 435667: Memory - corruptions (OVERRUN)
> >>> Overrunning buffer pointed to by "" of 4 bytes by passing
> it to a function which accesses it at byte offset 7.
> 88 const U64 hash = xxh64(, sizeof(U32), 0);
> 89 /* DDict ptr table size is a multiple of 2, use size - 1 as
> mask to get index within [0, hashSet->ddictPtrTableSize) */
> 90 return hash & (hashSet->ddictPtrTableSize - 1);
> 91 }
> 92
> 93 /* Adds DDict to a hashset without resizing it.
>
> ** CID 435666: Insecure data handling (TAINTED_SCALAR)
>
>
>
>
> *** CID 435666: Insecure data handling (TAINTED_SCALAR)
> /common/command.c: 674 in cmd_source_script()
> 668 ret = image_locate_script(buf, 0, fit_uname, confname,
> , );
> 669 unmap_sysmem(buf);
> 670 if (ret)
> 671 return CMD_RET_FAILURE;
> 672
> 673 debug("** Script length: %d\n", len);
> >>> CID 435666: Insecure data handling (TAINTED_SCALAR)
> >>> Passing tainted expression "len" to "run_command_list", which
[tom.r...@gmail.com: Fwd: New Defects reported by Coverity Scan for Das U-Boot]
Here's the latest report
-- Forwarded message -
From:
Date: Mon, Dec 5, 2022, 3:35 PM
Subject: New Defects reported by Coverity Scan for Das U-Boot
To:
Hi,
Please find the latest report on new defect(s) introduced to Das U-Boot
found with Coverity Scan.
4 new defect(s) introduced to Das U-Boot found with Coverity Scan.
1 defect(s), reported by Coverity Scan earlier, were marked fixed in the
recent build analyzed by Coverity Scan.
New defect(s) Reported-by: Coverity Scan
Showing 4 of 4 defect(s)
** CID 430977: Null pointer dereferences (FORWARD_NULL)
/net/ndisc.c: 268 in ndisc_receive()
*** CID 430977: Null pointer dereferences (FORWARD_NULL)
/net/ndisc.c: 268 in ndisc_receive()
262 sizeof(struct in6_addr)) == 0) &&
263 ndisc_has_option(ip6, ND_OPT_TARGET_LL_ADDR)) {
264 ndisc_extract_enetaddr(ndisc,
neigh_eth_addr);
265
266 /* save address for later use */
267 if (!net_nd_packet_mac)
>>> CID 430977: Null pointer dereferences (FORWARD_NULL)
>>> Passing null pointer "net_nd_packet_mac" to "memcpy", which
dereferences it. [Note: The source code implementation of the function has
been overridden by a builtin model.]
268 memcpy(net_nd_packet_mac,
neigh_eth_addr, 7);
269
270 /* modify header, and transmit it */
271 memcpy(((struct ethernet_hdr
*)net_nd_tx_packet)->et_dest,
272neigh_eth_addr, 6);
273
** CID 430976: Control flow issues (DEADCODE)
/net/tftp.c: 744 in sanitize_tftp_block_size_option()
*** CID 430976: Control flow issues (DEADCODE)
/net/tftp.c: 744 in sanitize_tftp_block_size_option()
738 }
739 /*
740 * If not CONFIG_IP_DEFRAG, cap at the same value as
741 * for tftp put, namely normal MTU minus protocol
742 * overhead.
743 */
>>> CID 430976: Control flow issues (DEADCODE)
>>> Execution cannot reach this statement: "[[fallthrough]];".
744 fallthrough;
745 case TFTPPUT:
746 default:
747 /*
748 * U-Boot does not support IP fragmentation on TX,
so
749 * this must be small enough that it fits normal MTU
** CID 430975: Control flow issues (MISSING_BREAK)
/net/net.c: 1270 in net_process_received_packet()
*** CID 430975: Control flow issues (MISSING_BREAK)
/net/net.c: 1270 in net_process_received_packet()
1264 #ifdef CONFIG_CMD_RARP
1265case PROT_RARP:
1266rarp_receive(ip, len);
1267break;
1268 #endif
1269 #if IS_ENABLED(CONFIG_IPV6)
>>> CID 430975: Control flow issues (MISSING_BREAK)
>>> The case for value "34525" is not terminated by a "break" statement.
1270case PROT_IP6:
1271net_ip6_handler(et, (struct ip6_hdr *)ip, len);
1272 #endif
1273case PROT_IP:
1274debug_cond(DEBUG_NET_PKT, "Got IP\n");
1275/* Before we start poking the header, make sure it
is there */
** CID 430974: Memory - corruptions (OVERRUN)
/net/ndisc.c: 268 in ndisc_receive()
*** CID 430974: Memory - corruptions (OVERRUN)
/net/ndisc.c: 268 in ndisc_receive()
262 sizeof(struct in6_addr)) == 0) &&
263 ndisc_has_option(ip6, ND_OPT_TARGET_LL_ADDR)) {
264 ndisc_extract_enetaddr(ndisc,
neigh_eth_addr);
265
266 /* save address for later use */
267 if (!net_nd_packet_mac)
>>> CID 430974: Memory - corruptions (OVERRUN)
>>> Overrunning array "neigh_eth_addr" of 6 bytes by passing it to a
function which accesses it at byte offset 6 using argument "7UL". [Note:
The source code implementation of the function has been overridden by a
builtin model.]
268 memcpy(net_nd_packet_mac,
neigh_eth_addr, 7);
269
270 /* modify header, and transmit it */
271 memcpy(((struct ethernet_hdr
*)net_nd_tx_packet)->et_dest,
272neigh_eth_addr, 6);
273
--
Tom
signature.asc
Description: PGP signature
Fwd: New Defects reported by Coverity Scan for Das U-Boot
Here's the latest report
-- Forwarded message -
From:
Date: Mon, Nov 21, 2022 at 12:44 PM
Subject: New Defects reported by Coverity Scan for Das U-Boot
To:
Hi,
Please find the latest report on new defect(s) introduced to Das
U-Boot found with Coverity Scan.
2 new defect(s) introduced to Das U-Boot found with Coverity Scan.
3 defect(s), reported by Coverity Scan earlier, were marked fixed in
the recent build analyzed by Coverity Scan.
New defect(s) Reported-by: Coverity Scan
Showing 2 of 2 defect(s)
** CID 376996: Error handling issues (CHECKED_RETURN)
/drivers/net/sandbox-raw-bus.c: 40 in eth_raw_bus_post_bind()
*** CID 376996: Error handling issues (CHECKED_RETURN)
/drivers/net/sandbox-raw-bus.c: 40 in eth_raw_bus_post_bind()
34 if (skip_localhost && local)
35 continue;
36
37 ub_ifname = calloc(IFNAMSIZ + sizeof(ub_ifname_pfx), 1);
38 strcpy(ub_ifname, ub_ifname_pfx);
39 strncat(ub_ifname, i->if_name, IFNAMSIZ);
>>> CID 376996: Error handling issues (CHECKED_RETURN)
>>> Calling "device_bind_driver" without checking return value (as is done
>>> elsewhere 12 out of 15 times).
40 device_bind_driver(dev, "eth_sandbox_raw", ub_ifname, );
41
42 device_set_name_alloced(child);
43 device_probe(child);
44 priv = dev_get_priv(child);
45 if (priv) {
** CID 376995: Null pointer dereferences (FORWARD_NULL)
/test/test-main.c: 518 in ut_run_tests()
*** CID 376995: Null pointer dereferences (FORWARD_NULL)
/test/test-main.c: 518 in ut_run_tests()
512 pos = dectoul(test_insert, NULL);
513 p = strchr(test_insert, ':');
514 if (p)
515 p++;
516
517 for (test = tests; test < tests + count; test++) {
>>> CID 376995: Null pointer dereferences (FORWARD_NULL)
>>> Passing null pointer "p" to "strcmp", which dereferences it. [Note: The
>>> source code implementation of the function has been overridden by a builtin
>>> model.]
518 if (!strcmp(p, test->name))
519 one = test;
520 }
521 }
522
523 for (upto = 0, test = tests; test < tests + count;
test++, upto++) {
--
Tom
signature.asc
Description: PGP signature
Fwd: New Defects reported by Coverity Scan for Das U-Boot
Here's the latest report.
-- Forwarded message -
From:
Date: Mon, Nov 7, 2022 at 3:41 PM
Subject: New Defects reported by Coverity Scan for Das U-Boot
To:
Hi,
Please find the latest report on new defect(s) introduced to Das
U-Boot found with Coverity Scan.
21 new defect(s) introduced to Das U-Boot found with Coverity Scan.
15 defect(s), reported by Coverity Scan earlier, were marked fixed in
the recent build analyzed by Coverity Scan.
New defect(s) Reported-by: Coverity Scan
Showing 20 of 21 defect(s)
** CID 376213: Memory - illegal accesses (UNINIT)
/lib/efi_loader/efi_boottime.c: 2642 in
efi_install_multiple_protocol_interfaces_int()
*** CID 376213: Memory - illegal accesses (UNINIT)
/lib/efi_loader/efi_boottime.c: 2642 in
efi_install_multiple_protocol_interfaces_int()
2636int i = 0;
2637efi_va_list argptr_copy;
2638
2639if (!handle)
2640return EFI_INVALID_PARAMETER;
2641
>>> CID 376213: Memory - illegal accesses (UNINIT)
>>> Using uninitialized value "argptr_copy" when calling
>>> "__builtin_ms_va_copy".
2642efi_va_copy(argptr_copy, argptr);
2643for (;;) {
2644protocol = efi_va_arg(argptr, efi_guid_t*);
2645if (!protocol)
2646break;
2647protocol_interface = efi_va_arg(argptr, void*);
** CID 376212: Error handling issues (CHECKED_RETURN)
*** CID 376212: Error handling issues (CHECKED_RETURN)
/drivers/usb/emul/sandbox_flash.c: 197 in handle_ufi_command()
191
192 ret = sb_scsi_emul_command(info, req, len);
193 if (!ret) {
194 setup_response(priv);
195 } else if ((ret == SCSI_EMUL_DO_READ || ret ==
SCSI_EMUL_DO_WRITE) &&
196priv->fd != -1) {
>>> CID 376212: Error handling issues (CHECKED_RETURN)
>>> Calling "os_lseek(priv->fd, info->seek_block * info->block_size, 0)"
>>> without checking return value. It wraps a library function that may fail
>>> and return an error code.
197 os_lseek(priv->fd, info->seek_block * info->block_size,
198 OS_SEEK_SET);
199 setup_response(priv);
200 } else {
201 setup_fail_response(priv);
202 }
** CID 376211:(TAINTED_SCALAR)
*** CID 376211:(TAINTED_SCALAR)
/cmd/eficonfig.c: 1475 in eficonfig_edit_boot_option()
1469if (lo.file_path)
1470fill_file_info(lo.file_path,
>file_info, device_dp);
1471
1472/* Initrd file path(optional) is placed at
second instance. */
1473initrd_dp = efi_dp_from_lo(, _lf2_initrd_guid);
1474if (initrd_dp) {
>>> CID 376211:(TAINTED_SCALAR)
>>> Passing tainted expression "initrd_dp->length" to "fill_file_info",
>>> which uses it as an offset.
1475fill_file_info(initrd_dp,
>initrd_info, initrd_device_dp);
1476efi_free_pool(initrd_dp);
1477}
1478
1479if (size > 0)
1480memcpy(bo->optional_data,
lo.optional_data, size);
/cmd/eficonfig.c: 1535 in eficonfig_edit_boot_option()
1529ret = eficonfig_set_boot_option(varname, final_dp,
final_dp_size, bo->description, tmp);
1530if (ret != EFI_SUCCESS)
1531goto out;
1532 out:
1533free(tmp);
1534free(bo->optional_data);
>>> CID 376211:(TAINTED_SCALAR)
>>> Passing tainted expression "*bo->description" to "dlfree", which uses
>>> it as an offset.
1535free(bo->description);
1536free(bo->file_info.current_path);
1537free(bo->initrd_info.current_path);
1538efi_free_pool(device_dp);
1539efi_free_pool(initrd_device_dp);
1540efi_free_pool(initrd_dp);
/cmd/eficonfig.c: 1534 in eficonfig_edit_boot_option()
1528
1529ret = eficonfig_set_boot_option(varname, final_dp,
final_dp_size, bo->description, tmp);
1530if (ret != EFI_SUCCESS)
1531goto out;
1532 out:
1533free(tmp);
>>> CID 376211:(TAINTED_SCALAR)
>>> Passing tainted expression "*bo->optional_data" to "dlfree", which uses
>>> it as an offset.
1534free(bo->optional_data);
1535free(bo->description);
1536free(bo->file_info.current_path);
1537free(bo->initrd_info.current_path);
1538efi_free_pool(device_dp);
1539
Re: [tom.r...@gmail.com: Fwd: New Defects reported by Coverity Scan for Das U-Boot]
Hi Tom,
On Wed, 24 Aug 2022 at 05:40, Tom Rini wrote:
>
> And here's the most recent one.
>
> - Forwarded message from Tom Rini -
>
> Date: Wed, 24 Aug 2022 07:38:55 -0400
> From: Tom Rini
> To: tr...@konsulko.com
> Subject: Fwd: New Defects reported by Coverity Scan for Das U-Boot
>
> -- Forwarded message -
> From:
> Date: Mon, Aug 22, 2022 at 7:07 PM
> Subject: New Defects reported by Coverity Scan for Das U-Boot
> To:
>
>
> Hi,
>
> Please find the latest report on new defect(s) introduced to Das
> U-Boot found with Coverity Scan.
>
> 3 new defect(s) introduced to Das U-Boot found with Coverity Scan.
> 2 defect(s), reported by Coverity Scan earlier, were marked fixed in
> the recent build analyzed by Coverity Scan.
>
> New defect(s) Reported-by: Coverity Scan
> Showing 3 of 3 defect(s)
>
>
> ** CID 356244: Null pointer dereferences (FORWARD_NULL)
>
>
>
> *** CID 356244: Null pointer dereferences (FORWARD_NULL)
> /boot/vbe.c: 46 in vbe_find_first_device()
> 40 int vbe_find_first_device(struct udevice **devp)
> 41 {
> 42 uclass_find_first_device(UCLASS_BOOTMETH, devp);
> 43 if (*devp && is_vbe(*devp))
> 44 return 0;
> 45
> >>> CID 356244: Null pointer dereferences (FORWARD_NULL)
> >>> Passing "devp" to "vbe_find_next_device", which dereferences null
> >>> "*devp".
> 46 return vbe_find_next_device(devp);
> 47 }
> 48
> 49 int vbe_list(void)
> 50 {
> 51 struct bootstd_priv *std;
>
> ** CID 356243: Code maintainability issues (UNUSED_VALUE)
> /boot/vbe_simple.c: 237 in bootmeth_vbe_simple_ft_fixup()
>
>
>
> *** CID 356243: Code maintainability issues (UNUSED_VALUE)
> /boot/vbe_simple.c: 237 in bootmeth_vbe_simple_ft_fixup()
> 231 /*
> 232 * Ideally we would have driver model support for
> fixups, but that does
> 233 * not exist yet. It is a step too far to try to do
> this before VBE is
> 234 * in place.
> 235 */
> 236 for (ret = vbe_find_first_device(); dev;
> >>> CID 356243: Code maintainability issues (UNUSED_VALUE)
> >>> Assigning value from "vbe_find_next_device()" to "ret" here, but
> >>> that stored value is overwritten before it can be used.
> 237 ret = vbe_find_next_device()) {
> 238 struct simple_state state;
> 239
> 240 if (strcmp("vbe_simple", dev->driver->name))
> 241 continue;
> 242
>
> ** CID 356242:(TAINTED_SCALAR)
>
>
>
> *** CID 356242:(TAINTED_SCALAR)
> /test/dm/ofnode.c: 501 in make_ofnode_fdt()
> 495 ut_assertok(fdt_end_node(fdt));
> 496
> 497 ut_assert(fdt_begin_node(fdt, "new-mmc") >= 0);
> 498 ut_assertok(fdt_end_node(fdt));
> 499
> 500 ut_assertok(fdt_end_node(fdt));
> >>> CID 356242:(TAINTED_SCALAR)
> >>> Passing tainted expression "fdt->size_dt_strings" to "fdt_finish",
> >>> which uses it as an offset.
> 501 ut_assertok(fdt_finish(fdt));
> 502
> 503 return 0;
> 504 }
> 505
> 506 static int dm_test_ofnode_root(struct unit_test_state *uts)
> /test/dm/ofnode.c: 501 in make_ofnode_fdt()
> 495 ut_assertok(fdt_end_node(fdt));
> 496
> 497 ut_assert(fdt_begin_node(fdt, "new-mmc") >= 0);
> 498 ut_assertok(fdt_end_node(fdt));
> 499
> 500 ut_assertok(fdt_end_node(fdt));
> >>> CID 356242:(TAINTED_SCALAR)
> >>> Passing tainted expression "fdt->size_dt_strings" to "fdt_finish",
> >>> which uses it as an offset.
> 501 ut_assertok(fdt_finish(fdt));
> 502
> 503 return 0;
> 504 }
> 505
> 506 static int dm_test_ofnode_root(struct unit_test_state *uts)
>
>
>
> To view the defects in Coverity Scan visit,
> https://u15810271.ct.sendgrid.net/ls/click?upn=HRESupC-2F2Czv4BOaCWWCy7my0P0qcxCbhZ31OYv50yoA22WlOQ-2By3ieUvdb
[tom.r...@gmail.com: Fwd: New Defects reported by Coverity Scan for Das U-Boot]
And here's the most recent one.
- Forwarded message from Tom Rini -
Date: Wed, 24 Aug 2022 07:38:55 -0400
From: Tom Rini
To: tr...@konsulko.com
Subject: Fwd: New Defects reported by Coverity Scan for Das U-Boot
-- Forwarded message -
From:
Date: Mon, Aug 22, 2022 at 7:07 PM
Subject: New Defects reported by Coverity Scan for Das U-Boot
To:
Hi,
Please find the latest report on new defect(s) introduced to Das
U-Boot found with Coverity Scan.
3 new defect(s) introduced to Das U-Boot found with Coverity Scan.
2 defect(s), reported by Coverity Scan earlier, were marked fixed in
the recent build analyzed by Coverity Scan.
New defect(s) Reported-by: Coverity Scan
Showing 3 of 3 defect(s)
** CID 356244: Null pointer dereferences (FORWARD_NULL)
*** CID 356244: Null pointer dereferences (FORWARD_NULL)
/boot/vbe.c: 46 in vbe_find_first_device()
40 int vbe_find_first_device(struct udevice **devp)
41 {
42 uclass_find_first_device(UCLASS_BOOTMETH, devp);
43 if (*devp && is_vbe(*devp))
44 return 0;
45
>>> CID 356244: Null pointer dereferences (FORWARD_NULL)
>>> Passing "devp" to "vbe_find_next_device", which dereferences null
>>> "*devp".
46 return vbe_find_next_device(devp);
47 }
48
49 int vbe_list(void)
50 {
51 struct bootstd_priv *std;
** CID 356243: Code maintainability issues (UNUSED_VALUE)
/boot/vbe_simple.c: 237 in bootmeth_vbe_simple_ft_fixup()
*** CID 356243: Code maintainability issues (UNUSED_VALUE)
/boot/vbe_simple.c: 237 in bootmeth_vbe_simple_ft_fixup()
231 /*
232 * Ideally we would have driver model support for
fixups, but that does
233 * not exist yet. It is a step too far to try to do
this before VBE is
234 * in place.
235 */
236 for (ret = vbe_find_first_device(); dev;
>>> CID 356243: Code maintainability issues (UNUSED_VALUE)
>>> Assigning value from "vbe_find_next_device()" to "ret" here, but
>>> that stored value is overwritten before it can be used.
237 ret = vbe_find_next_device()) {
238 struct simple_state state;
239
240 if (strcmp("vbe_simple", dev->driver->name))
241 continue;
242
** CID 356242:(TAINTED_SCALAR)
*** CID 356242:(TAINTED_SCALAR)
/test/dm/ofnode.c: 501 in make_ofnode_fdt()
495 ut_assertok(fdt_end_node(fdt));
496
497 ut_assert(fdt_begin_node(fdt, "new-mmc") >= 0);
498 ut_assertok(fdt_end_node(fdt));
499
500 ut_assertok(fdt_end_node(fdt));
>>> CID 356242:(TAINTED_SCALAR)
>>> Passing tainted expression "fdt->size_dt_strings" to "fdt_finish",
>>> which uses it as an offset.
501 ut_assertok(fdt_finish(fdt));
502
503 return 0;
504 }
505
506 static int dm_test_ofnode_root(struct unit_test_state *uts)
/test/dm/ofnode.c: 501 in make_ofnode_fdt()
495 ut_assertok(fdt_end_node(fdt));
496
497 ut_assert(fdt_begin_node(fdt, "new-mmc") >= 0);
498 ut_assertok(fdt_end_node(fdt));
499
500 ut_assertok(fdt_end_node(fdt));
>>> CID 356242:(TAINTED_SCALAR)
>>> Passing tainted expression "fdt->size_dt_strings" to "fdt_finish",
>>> which uses it as an offset.
501 ut_assertok(fdt_finish(fdt));
502
503 return 0;
504 }
505
506 static int dm_test_ofnode_root(struct unit_test_state *uts)
To view the defects in Coverity Scan visit,
https://u15810271.ct.sendgrid.net/ls/click?upn=HRESupC-2F2Czv4BOaCWWCy7my0P0qcxCbhZ31OYv50yoA22WlOQ-2By3ieUvdbKmOyw68TMVT4Kip-2BBzfOGWXJ5yIiYplmPF9KAnKIja4Zd7tU-3Dl_S3_EEm8SbLgSDsaDZif-2Bv7ch8WqhKpLoKErHi4nXpwDNTu-2FviBcJy3TYnkbff9O1lpJB2a065UniCzfVIBu-2Brs6HGPrhp6hp3s-2BQGSVvNSaRsQojbpJAi7kxyFcHZ8aaIeQ0LJlzM2cTXzCCeq8c-2FquCeg4mCmdPzUFdWUhBcgytnExm8LYbWctf-2B-2BcK49gD2uvdO0dVdoZGeFYKdAJZGcKrg-3D-3D
To manage Coverity Scan email notifications for
"tom.r...@gmail.com", click
https://u15810271.ct.sendgrid.net/ls/click?upn=HRESupC-2F2Czv4BOaCWWCy7my0P0qcxCbhZ31OYv50yped04pjJnmXOsUBtKYNIXxWeIHzDeopm-2BEWQ6S6K-2FtUHv9ZTk8qZbuzkkz9sa-2BJFzf226DuRd-2B2ygQlLnerl-2BA3jN1AOYejXZ-2FNZ62waJHedPFGpqqjTx8fawy9KPJBno-3D0xWA_EEm8SbLgSDsaDZ
[tom.r...@gmail.com: Fwd: New Defects reported by Coverity Scan for Das U-Boot]
A bit behind on forwarding these along.
- Forwarded message from Tom Rini -
Date: Wed, 24 Aug 2022 07:38:46 -0400
From: Tom Rini
To: tr...@konsulko.com
Subject: Fwd: New Defects reported by Coverity Scan for Das U-Boot
-- Forwarded message -
From:
Date: Mon, Aug 8, 2022 at 8:51 PM
Subject: New Defects reported by Coverity Scan for Das U-Boot
To:
Hi,
Please find the latest report on new defect(s) introduced to Das
U-Boot found with Coverity Scan.
6 new defect(s) introduced to Das U-Boot found with Coverity Scan.
2 defect(s), reported by Coverity Scan earlier, were marked fixed in
the recent build analyzed by Coverity Scan.
New defect(s) Reported-by: Coverity Scan
Showing 6 of 6 defect(s)
** CID 355771:(PRINTF_ARGS)
*** CID 355771:(PRINTF_ARGS)
/test/cmd/fdt.c: 72 in fdt_test_addr()
66 ut_assertok(run_command("fdt addr", 0));
67 ut_assert_nextline("Working fdt: %08lx", (ulong)map_to_sysmem(fdt));
68 ut_assertok(ut_check_console_end(uts));
69
70 /* Set the working FDT */
71 set_working_fdt_addr(0);
>>> CID 355771:(PRINTF_ARGS)
>>> Argument "addr" to format specifier "%08x" was expected to have type
>>> "unsigned int" but has type "unsigned long".
72 ut_assertok(run_commandf("fdt addr %08x", addr));
73 ut_asserteq(addr, map_to_sysmem(working_fdt));
74 ut_assertok(ut_check_console_end(uts));
75 set_working_fdt_addr(0);
76
77 /* Set the working FDT */
/test/cmd/fdt.c: 89 in fdt_test_addr()
83 ut_assertok(ret);
84 ut_asserteq(addr, map_to_sysmem(new_fdt));
85 ut_assertok(ut_check_console_end(uts));
86
87 /* Test setting an invalid FDT */
88 fdt[0] = 123;
>>> CID 355771:(PRINTF_ARGS)
>>> Argument "addr" to format specifier "%08x" was expected to have type
>>> "unsigned int" but has type "unsigned long".
89 ut_asserteq(1, run_commandf("fdt addr %08x", addr));
90 ut_assert_nextline("libfdt fdt_check_header(): FDT_ERR_BADMAGIC");
91 ut_assertok(ut_check_console_end(uts));
92
93 /* Test detecting an invalid FDT */
94 fdt[0] = 123;
/test/cmd/fdt.c: 80 in fdt_test_addr()
74 ut_assertok(ut_check_console_end(uts));
75 set_working_fdt_addr(0);
76
77 /* Set the working FDT */
78 fdt_blob = gd->fdt_blob;
79 gd->fdt_blob = NULL;
>>> CID 355771:(PRINTF_ARGS)
>>> Argument "addr" to format specifier "%08x" was expected to have type
>>> "unsigned int" but has type "unsigned long".
80 ret = run_commandf("fdt addr -c %08x", addr);
81 new_fdt = gd->fdt_blob;
82 gd->fdt_blob = fdt_blob;
83 ut_assertok(ret);
84 ut_asserteq(addr, map_to_sysmem(new_fdt));
85 ut_assertok(ut_check_console_end(uts));
** CID 355770: Insecure data handling (TAINTED_SCALAR)
*** CID 355770: Insecure data handling (TAINTED_SCALAR)
/test/cmd/fdt.c: 37 in make_test_fdt()
31 static int make_test_fdt(struct unit_test_state *uts, void
*fdt, int size)
32 {
33 ut_assertok(fdt_create(fdt, size));
34 ut_assertok(fdt_finish_reservemap(fdt));
35 ut_assert(fdt_begin_node(fdt, "") >= 0);
36 ut_assertok(fdt_end_node(fdt));
>>> CID 355770: Insecure data handling (TAINTED_SCALAR)
>>> Passing tainted expression "fdt->size_dt_strings" to "fdt_finish",
>>> which uses it as an offset.
37 ut_assertok(fdt_finish(fdt));
38
39 return 0;
40 }
41
42 /* Test 'fdt addr' getting/setting address */
** CID 355769:(PRINTF_ARGS)
*** CID 355769:(PRINTF_ARGS)
/test/cmd/fdt.c: 121 in fdt_test_resize()
115 /* Test setting and resizing the working FDT to a larger size */
116 ut_assertok(console_record_reset_enable());
117 ut_assertok(run_commandf("fdt addr %08x %x", addr, newsize));
118 ut_assertok(ut_check_console_end(uts));
119
120 /* Try shrinking it */
>>> CID 355769:(PRINTF_ARGS)
>>> Argument "addr" to format specifier "%08x" was expected to have type
>>> "unsigned int" but has type "unsigned long".
121 ut_assertok(run_commandf("fdt addr %08x %x", addr,
sizeof(fdt) / 4));
122 ut_assert_nextline("New length %d < existing length
%d, ignoring",
123
Fwd: New Defects reported by Coverity Scan for Das U-Boot
Hello Tom,
could you, please, have a look at the problems reported by Coverity
concerning code introduced by you into U-Boot.
For SHA256_Update_recycle() I guess you just have to change the
signature of the function to
SHA256_Update_recycled (SHA256_CTX *ctx,
unsigned char *block, size_t len)
Looking at
https://scan8.scan.coverity.com/reports.htm#v40863/p10710/fileInstanceId=59559157=12260012=355364
https://scan8.scan.coverity.com/reports.htm#v40863/p10710/fileInstanceId=59559157=12260012=355365
and
https://scan8.scan.coverity.com/reports.htm#v40863/p10710/fileInstanceId=59559157=12260012=355366
I think the issues are false positives:
Coverity ignores that if the sha256_update() is called will length < 64
sha256_process() will be called with blocks = 0 and will not access the
buffer.
Best regards
Heinrich
Forwarded Message
Subject: New Defects reported by Coverity Scan for Das U-Boot
Date: Tue, 26 Jul 2022 00:49:17 + (UTC)
From: scan-ad...@coverity.com
To: xypron.g...@gmx.de
Hi,
Please find the latest report on new defect(s) introduced to Das U-Boot
found with Coverity Scan.
3 new defect(s) introduced to Das U-Boot found with Coverity Scan.
2 defect(s), reported by Coverity Scan earlier, were marked fixed in the
recent build analyzed by Coverity Scan.
New defect(s) Reported-by: Coverity Scan
Showing 3 of 3 defect(s)
** CID 355366:(OVERRUN)
*** CID 355366:(OVERRUN)
/lib/crypt/crypt-sha256.c: 104 in SHA256_Update_recycled()
98 SHA256_Update_recycled (SHA256_CTX *ctx,
99 unsigned char block[32], size_t len)
100 {
101 size_t cnt;
102 for (cnt = len; cnt >= 32; cnt -= 32)
103 SHA256_Update (ctx, block, 32);
CID 355366:(OVERRUN)
Overrunning buffer pointed to by "(void const *)block" of 32 bytes by
passing it to a function which accesses it at byte offset 63.
104 SHA256_Update (ctx, block, cnt);
105 }
106 107 void
108 crypt_sha256crypt_rn (const char *phrase, size_t phr_size,
109 const char *setting, size_t ARG_UNUSED
(set_size),
/lib/crypt/crypt-sha256.c: 103 in SHA256_Update_recycled()
97 static void
98 SHA256_Update_recycled (SHA256_CTX *ctx,
99 unsigned char block[32], size_t len)
100 {
101 size_t cnt;
102 for (cnt = len; cnt >= 32; cnt -= 32)
CID 355366:(OVERRUN)
Overrunning buffer pointed to by "(void const *)block" of 32 bytes by
passing it to a function which accesses it at byte offset 63.
103 SHA256_Update (ctx, block, 32);
104 SHA256_Update (ctx, block, cnt);
105 }
106 107 void
108 crypt_sha256crypt_rn (const char *phrase, size_t phr_size,
** CID 355365: Memory - corruptions (OVERRUN)
*** CID 355365: Memory - corruptions (OVERRUN)
/lib/crypt/crypt-sha256.c: 212 in crypt_sha256crypt_rn()
206 characters and it ends at the first `$' character (for
207 compatibility with existing implementations). */
208 SHA256_Update (ctx, salt, salt_size);
209 210 /* Add for any character in the phrase one byte of the
alternate sum. */
211 for (cnt = phr_size; cnt > 32; cnt -= 32)
CID 355365: Memory - corruptions (OVERRUN)
Overrunning buffer pointed to by "(void const *)result" of 32 bytes by
passing it to a function which accesses it at byte offset 63.
212 SHA256_Update (ctx, result, 32);
213 SHA256_Update (ctx, result, cnt);
214 215 /* Take the binary representation of the length of the
phrase and for every
216 1 add the alternate sum, for every 0 the phrase. */
217 for (cnt = phr_size; cnt > 0; cnt >>= 1)
** CID 355364:(OVERRUN)
*** CID 355364:(OVERRUN)
/lib/sha256.c: 259 in sha256_finish()
253 PUT_UINT32_BE(low, msglen, 4);
254 255 last = ctx->total[0] & 0x3F;
256 padn = (last < 56) ? (56 - last) : (120 - last);
257 258 sha256_update(ctx, sha256_padding, padn);
CID 355364:(OVERRUN)
Overrunning array "msglen" of 8 bytes by passing it to a function which
accesses it at byte offset 63.
259 sha256_update(ctx, msglen, 8);
260 261 PUT_UINT32_BE(ctx->state[0], digest, 0);
262 PUT_UINT32_BE(ctx->state[1], digest, 4);
263 PUT_UINT32_BE(ctx->state[2], digest, 8);
264 PUT_UINT32_BE(ctx->state[3], digest, 12);
/lib/sha256.c: 259 in sha256_finish()
253 PUT_UINT32_BE(low, msglen, 4);
254 255 last = ctx->total[0] & 0x3F;
256 padn = (last <
Re: Fwd: New Defects reported by Coverity Scan for Das U-Boot
On Tue, Aug 17, 2021 at 07:21:43AM +0200, Heinrich Schuchardt wrote: > Hello Tom, > > I suggest to mark these as invalid: > > CID 338485 > CID 338490 > CID 338489 Done, thanks. -- Tom signature.asc Description: PGP signature
Fwd: New Defects reported by Coverity Scan for Das U-Boot
Hello Tom,
I suggest to mark these as invalid:
CID 338485
CID 338490
CID 338489
Best regards
Heinrich
Forwarded Message
Subject: New Defects reported by Coverity Scan for Das U-Boot
Date: Mon, 16 Aug 2021 18:33:23 + (UTC)
From: scan-ad...@coverity.com
To: xypron.g...@gmx.de
Hi,
Please find the latest report on new defect(s) introduced to Das U-Boot
found with Coverity Scan.
7 new defect(s) introduced to Das U-Boot found with Coverity Scan.
3 defect(s), reported by Coverity Scan earlier, were marked fixed in the
recent build analyzed by Coverity Scan.
New defect(s) Reported-by: Coverity Scan
Showing 7 of 7 defect(s)
** CID 338491: Null pointer dereferences (NULL_RETURNS)
/tools/kwbimage.c: 1066 in export_pub_kak_hash()
*** CID 338491: Null pointer dereferences (NULL_RETURNS)
/tools/kwbimage.c: 1066 in export_pub_kak_hash()
1060int res;
1061 1062 hashf = fopen("pub_kak_hash.txt", "w");
1063 1064 res = kwb_export_pubkey(kak, _hdr->kak, hashf,
"KAK");
1065 >>> CID 338491: Null pointer dereferences (NULL_RETURNS)
Dereferencing a pointer that might be "NULL" "hashf" when calling "fclose".
1066fclose(hashf);
1067 1068 return res < 0 ? 1 : 0;
1069 }
1070 1071 int kwb_sign_csk_with_kak(struct image_tool_params
*params,
** CID 338490: Control flow issues (DEADCODE)
/drivers/tpm/sandbox_common.c: 34 in sb_tpm_index_to_seq()
*** CID 338490: Control flow issues (DEADCODE)
/drivers/tpm/sandbox_common.c: 34 in sb_tpm_index_to_seq()
28 case FWMP_NV_INDEX:
29 return NV_SEQ_FWMP;
30 case MRC_REC_HASH_NV_INDEX:
31 return NV_SEQ_REC_HASH;
32 case 0:
33 return NV_SEQ_GLOBAL_LOCK;
CID 338490: Control flow issues (DEADCODE)
Execution cannot reach this statement: "case TPM_NV_INDEX_LOCK:".
34 case TPM_NV_INDEX_LOCK:
35 return NV_SEQ_ENABLE_LOCKING;
36 }
37 38 printf("Invalid nv index %#x\n", index);
39 return -1;
** CID 338489: Control flow issues (DEADCODE)
/drivers/tpm/tpm2_tis_sandbox.c: 652 in sandbox_tpm2_xfer()
*** CID 338489: Control flow issues (DEADCODE)
/drivers/tpm/tpm2_tis_sandbox.c: 652 in sandbox_tpm2_xfer()
646 647 for (i = 0; i < SANDBOX_TPM_PCR_NB; i++)
648 if (pcr_map & BIT(i))
649 pcr_index = i;
650 651 if (pcr_index >= SANDBOX_TPM_PCR_NB) {
CID 338489: Control flow issues (DEADCODE)
Execution cannot reach this statement: "printf("Invalid index %d, s...".
652 printf("Invalid index %d, sandbox TPM handles up to
%d PCR(s)\n",
653pcr_index, SANDBOX_TPM_PCR_NB);
654 rc = TPM2_RC_VALUE;
655 return sandbox_tpm2_fill_buf(recv, recv_len,
tag, rc);
656 }
657
** CID 338488: Memory - illegal accesses (NEGATIVE_RETURNS)
/tools/kwbimage.c: 1093 in kwb_sign_csk_with_kak()
*** CID 338488: Memory - illegal accesses (NEGATIVE_RETURNS)
/tools/kwbimage.c: 1093 in kwb_sign_csk_with_kak()
1087if (export_pub_kak_hash(kak, secure_hdr))
1088return 1;
1089 1090 if (kwb_import_pubkey(_pub, _hdr->kak,
"KAK") < 0)
1091return 1;
1092 >>> CID 338488: Memory - illegal accesses (NEGATIVE_RETURNS)
Using variable "csk_idx" as an index to array "secure_hdr->csk".
1093if (kwb_export_pubkey(csk, _hdr->csk[csk_idx], NULL,
"CSK") < 0)
1094return 1;
1095 1096 if (kwb_sign_and_verify(kak, _hdr->csk,
1097sizeof(secure_hdr->csk) +
1098sizeof(secure_hdr->csksig),
** CID 338487: Null pointer dereferences (FORWARD_NULL)
*** CID 338487: Null pointer dereferences (FORWARD_NULL)
/test/dm/ecdsa.c: 34 in dm_test_ecdsa_verify()
28 struct image_sign_info info = {
29 .checksum = ,
30 };
31 32 ut_assertok(uclass_get(UCLASS_ECDSA, ));
33 ut_assertnonnull(ucp);
CID 338487: Null pointer dereferences (FORWARD_NULL)
Passing "" to "ecdsa_verify", which dereferences null "info.fdt_blob".
34 ut_asserteq(-ENODEV, ecdsa_verify(, NULL, 0, NULL, 0));
35 36 return 0;
37 }
**
Fwd: New Defects reported by Coverity Scan for Das U-Boot
On 4/23/21 6:38 PM, scan-ad...@coverity.com wrote:
Hi,
Please find the latest report on new defect(s) introduced to Das U-Boot found
with Coverity Scan.
3 new defect(s) introduced to Das U-Boot found with Coverity Scan.
New defect(s) Reported-by: Coverity Scan
Showing 3 of 3 defect(s)
** CID 331185: Insecure data handling (TAINTED_SCALAR)
/lib/lz4.c: 143 in LZ4_decompress_generic()
*** CID 331185: Insecure data handling (TAINTED_SCALAR)
/lib/lz4.c: 143 in LZ4_decompress_generic()
137 }
138 else
139 {
140 if ((!endOnInput) && (cpy != oend)) goto _output_error;
/* Error : block decoding must stop exactly there */
141 if ((endOnInput) && ((ip+length != iend) || (cpy >
oend))) goto _output_error; /* Error : input must be consumed */
142 }
CID 331185: Insecure data handling (TAINTED_SCALAR)
Passing tainted variable "length" to a tainted sink. [Note: The source
code implementation of the function has been overridden by a builtin model.]
143 memcpy(op, ip, length);
144 ip += length;
145 op += length;
146 break; /* Necessarily EOF, due to parsing restrictions
*/
147 }
148 LZ4_wildCopy(op, ip, cpy);
** CID 331184: Memory - corruptions (OVERRUN)
/cmd/stackprot_test.c: 14 in do_test_stackprot_fail()
*** CID 331184: Memory - corruptions (OVERRUN)
/cmd/stackprot_test.c: 14 in do_test_stackprot_fail()
8
9 static int do_test_stackprot_fail(struct cmd_tbl *cmdtp, int flag, int
argc,
10char *const argv[])
11 {
Hello Tom,
please, mark this finding as intentional in Coverity.
12 char a[128];
13
CID 331184: Memory - corruptions (OVERRUN)
Overrunning array "a" of 128 bytes by passing it to a function which accesses it at
byte offset 511 using argument "512UL". [Note: The source code implementation of the
function has been overridden by a builtin model.]
14 memset(a, 0xa5, 512);
15 return 0;
16 }
17
18 U_BOOT_CMD(stackprot_test, 1, 1, do_test_stackprot_fail,
** CID 331183: Memory - corruptions (BUFFER_SIZE)
/cmd/stackprot_test.c: 14 in do_test_stackprot_fail()
*** CID 331183: Memory - corruptions (BUFFER_SIZE)
/cmd/stackprot_test.c: 14 in do_test_stackprot_fail()
same here
Best regards
Heinrich
8
9 static int do_test_stackprot_fail(struct cmd_tbl *cmdtp, int flag, int
argc,
10char *const argv[])
11 {
12 char a[128];
13
CID 331183: Memory - corruptions (BUFFER_SIZE)
You might overrun the 128 byte destination string "a" by writing the maximum 512
bytes from "165".
14 memset(a, 0xa5, 512);
15 return 0;
16 }
17
18 U_BOOT_CMD(stackprot_test, 1, 1, do_test_stackprot_fail,
To view the defects in Coverity Scan visit,
https://u15810271.ct.sendgrid.net/ls/click?upn=HRESupC-2F2Czv4BOaCWWCy7my0P0qcxCbhZ31OYv50yoA22WlOQ-2By3ieUvdbKmOyw68TMVT4Kip-2BBzfOGWXJ5yIiYplmPF9KAnKIja4Zd7tU-3DVLO3_N64QlSHam5hYYsLU0uvEm3xiMtcSlv2JwRoKVmjv-2F2X9PIw0aqIVMZlR6cmf9w8prU0ddkFkhQg-2B6p8UvlY-2FM51TBl-2FigNKw0KCrquAEkBb2jC3ZnWBwbVEZhLkDdq-2FMFkIpcluF4NvkPbaQ8l7PMYWmxLPqhtFLo01zbJ6O05zRrW9MzeWZiF82fugYqxJUGlLrQGmeTLuFDr2CDzEGJg-3D-3D
To manage Coverity Scan email notifications for "xypron.g...@gmx.de", click
https://u15810271.ct.sendgrid.net/ls/click?upn=HRESupC-2F2Czv4BOaCWWCy7my0P0qcxCbhZ31OYv50yped04pjJnmXOsUBtKYNIXx4Y-2F1WK-2FIlbEOzfoxXLI-2FdwA0wwGn90rGGrBgiHW-2ByLDLbUOEV7XOvtc9zJmj9LPyrT06WSaMnNrm6wfrUN-2BXuWoaHdqOoEyL7CQlGSiE-2BfE-3DtDQo_N64QlSHam5hYYsLU0uvEm3xiMtcSlv2JwRoKVmjv-2F2X9PIw0aqIVMZlR6cmf9w8pA8-2FW82eD6YTWlxlNXjrDSc-2B-2BfgU0QJMdYPvNOg-2Brk8a8VMShB-2FvhmE5GTrUF2ImOx4sRousy5Sh2qX6apgHec8wEC6ZWvhuro1Ua3CVllqnKzeW-2FmUepM3XfZqtYssGH0ujkCtgvKvxZfYpXxJdKdg-3D-3D
Re: Fwd: New Defects reported by Coverity Scan for Das U-Boot
On Tue, May 26, 2020 at 10:36:44PM +0200, Heinrich Schuchardt wrote:
> On 26.05.20 22:10, Tom Rini wrote:
> > On Tue, May 26, 2020 at 10:02:36PM +0200, Heinrich Schuchardt
> > wrote:
> >> On 26.05.20 20:40, Tom Rini wrote:
> >>> Ah, I thought you might not have been part of Coverity.
> >>> https://scan.coverity.com/projects/das-u-boot is where to
> >>> start, it will take GitHub credentials and then I can approve
> >>> you.
> >>
> >> Thanks for granting access. In the GUI one can really drill down
> >> into the explanation of the problem. This is very helpful.
> >
> > And thanks for digging more!
> >
> >>
> >>> ** CID 303760:(TAINTED_SCALAR)
> >>>
> >>>
> >>>
> >>
> >>>
> >>
> *** CID 303760:(TAINTED_SCALAR)
> >>> /cmd/efidebug.c: 938 in show_efi_boot_order() 932
> >>> } 933 p = label; 934
> >>> utf16_utf8_strncpy(, lo.label, label_len16); 935
> >>> printf("%2d: %s: %s\n", i + 1, var_name, label); 936
> >>> free(label); 937
> >> CID 303760:(TAINTED_SCALAR) Passing tainted variable
> >> "data" to a tainted sink.
> >>> 938 free(data); 939 } 940
> >>> out: 941 free(bootorder); 942 943
> >>> return ret; /cmd/efidebug.c: 929 in show_efi_boot_order() 923
> >>> efi_deserialize_load_option(, data); 924 925
> >>> label_len16 = u16_strlen(lo.label); 926
> >>> label_len = utf16_utf8_strnlen(lo.label,
> >> label_len16);
> >>> 927 label = malloc(label_len + 1); 928
> >>> if (!label) {
> >> CID 303760:(TAINTED_SCALAR) Passing tainted variable
> >> "data" to a tainted sink.
> >>> 929 free(data); 930
> >>> ret = CMD_RET_FAILURE; 931 goto
> >>> out; 932 } 933 p =
> >>> label; 934 utf16_utf8_strncpy(, lo.label,
> >>> label_len16);
> >>
> >> In CID 303760 the logic is as follows:
> >>
> >> In show_efi_boot_order() we malloc() memory. The allocated buffer
> >> is filled via byte swapping (get_unaligned_le16(),
> >> get_unaligned_le32()).
> >>
> >> Here comes Coverity's logic: "byte_swapping: Performing a byte
> >> swapping operation on ... implies that it came from an external
> >> source, and is therefore tainted."
> >>
> >> Now we pass the pointer to free(). Free() looks at 16 bytes
> >> preceding the pointer. Therefore free() is considered a tainted
> >> sink and an issue is raised.
> >>
> >> https://community.synopsys.com/s/article/From-Case-Clearing-TAINTED-STRING
> >>
> >>
> suggests to use Coverity specific comments to mark cleansing functions.
> >> This is not what I am inclined to do.
> >>
> >> CCing Takahiro as he had a hand in this code.
> >
> > So, option B on that link is what I was thinking about which is
> > creating a function in the model file to tell Coverity it's
> > handled. I was going to include what ours was already as I thought
> > I had written one, but there's not one in the dashboard currently.
> > And frankly a drawback of Coverity is that you can't iterate on
> > testing those kind of things easily.
>
> Here are example model files for Coverity:
>
> https://github.com/qemu/qemu/blob/master/scripts/coverity-model.c
> https://github.com/python/cpython/blob/master/Misc/coverity_model.c
>
> How many functions do you think we will have to maintain in the model
> file? Who will take the effort?
Ah yes, I think I looked at those a while ago and didn't come up with
anything that reduced our defects so I set it aside to look harder at
later. And haven't yet cycled back.
I would say once we have an initial functional skeleton in, any time
someone sees a Coverity defect that's a false positive and wants to
write a model function to cover it rather than just close it out in the
dashboard, we'll get an update to it and I'll push it to Coverity.
--
Tom
signature.asc
Description: PGP signature
Re: Fwd: New Defects reported by Coverity Scan for Das U-Boot
On 26.05.20 22:10, Tom Rini wrote:
> On Tue, May 26, 2020 at 10:02:36PM +0200, Heinrich Schuchardt
> wrote:
>> On 26.05.20 20:40, Tom Rini wrote:
>>> Ah, I thought you might not have been part of Coverity.
>>> https://scan.coverity.com/projects/das-u-boot is where to
>>> start, it will take GitHub credentials and then I can approve
>>> you.
>>
>> Thanks for granting access. In the GUI one can really drill down
>> into the explanation of the problem. This is very helpful.
>
> And thanks for digging more!
>
>>
>>> ** CID 303760:(TAINTED_SCALAR)
>>>
>>>
>>>
>>
>>>
>>
*** CID 303760:(TAINTED_SCALAR)
>>> /cmd/efidebug.c: 938 in show_efi_boot_order() 932
>>> } 933 p = label; 934
>>> utf16_utf8_strncpy(, lo.label, label_len16); 935
>>> printf("%2d: %s: %s\n", i + 1, var_name, label); 936
>>> free(label); 937
>> CID 303760:(TAINTED_SCALAR) Passing tainted variable
>> "data" to a tainted sink.
>>> 938 free(data); 939 } 940
>>> out: 941 free(bootorder); 942 943
>>> return ret; /cmd/efidebug.c: 929 in show_efi_boot_order() 923
>>> efi_deserialize_load_option(, data); 924 925
>>> label_len16 = u16_strlen(lo.label); 926
>>> label_len = utf16_utf8_strnlen(lo.label,
>> label_len16);
>>> 927 label = malloc(label_len + 1); 928
>>> if (!label) {
>> CID 303760:(TAINTED_SCALAR) Passing tainted variable
>> "data" to a tainted sink.
>>> 929 free(data); 930
>>> ret = CMD_RET_FAILURE; 931 goto
>>> out; 932 } 933 p =
>>> label; 934 utf16_utf8_strncpy(, lo.label,
>>> label_len16);
>>
>> In CID 303760 the logic is as follows:
>>
>> In show_efi_boot_order() we malloc() memory. The allocated buffer
>> is filled via byte swapping (get_unaligned_le16(),
>> get_unaligned_le32()).
>>
>> Here comes Coverity's logic: "byte_swapping: Performing a byte
>> swapping operation on ... implies that it came from an external
>> source, and is therefore tainted."
>>
>> Now we pass the pointer to free(). Free() looks at 16 bytes
>> preceding the pointer. Therefore free() is considered a tainted
>> sink and an issue is raised.
>>
>> https://community.synopsys.com/s/article/From-Case-Clearing-TAINTED-STRING
>>
>>
suggests to use Coverity specific comments to mark cleansing functions.
>> This is not what I am inclined to do.
>>
>> CCing Takahiro as he had a hand in this code.
>
> So, option B on that link is what I was thinking about which is
> creating a function in the model file to tell Coverity it's
> handled. I was going to include what ours was already as I thought
> I had written one, but there's not one in the dashboard currently.
> And frankly a drawback of Coverity is that you can't iterate on
> testing those kind of things easily.
Here are example model files for Coverity:
https://github.com/qemu/qemu/blob/master/scripts/coverity-model.c
https://github.com/python/cpython/blob/master/Misc/coverity_model.c
How many functions do you think we will have to maintain in the model
file? Who will take the effort?
Best regards
Heinrich
>
> Option C is to just mark this (and the similar ones you can see via
> the dashboard) as false positive.
>
Re: Fwd: New Defects reported by Coverity Scan for Das U-Boot
On Tue, May 26, 2020 at 10:02:36PM +0200, Heinrich Schuchardt wrote:
> On 26.05.20 20:40, Tom Rini wrote:
> > Ah, I thought you might not have been part of Coverity.
> > https://scan.coverity.com/projects/das-u-boot is where to start, it will
> > take GitHub credentials and then I can approve you.
>
> Thanks for granting access. In the GUI one can really drill down into
> the explanation of the problem. This is very helpful.
And thanks for digging more!
>
> > ** CID 303760:(TAINTED_SCALAR)
> >
> >
> >
>
> > *** CID 303760:(TAINTED_SCALAR)
> > /cmd/efidebug.c: 938 in show_efi_boot_order()
> > 932 }
> > 933 p = label;
> > 934 utf16_utf8_strncpy(, lo.label, label_len16);
> > 935 printf("%2d: %s: %s\n", i + 1, var_name, label);
> > 936 free(label);
> > 937
> CID 303760:(TAINTED_SCALAR)
> Passing tainted variable "data" to a tainted sink.
> > 938 free(data);
> > 939 }
> > 940 out:
> > 941 free(bootorder);
> > 942
> > 943 return ret;
> > /cmd/efidebug.c: 929 in show_efi_boot_order()
> > 923 efi_deserialize_load_option(, data);
> > 924
> > 925 label_len16 = u16_strlen(lo.label);
> > 926 label_len = utf16_utf8_strnlen(lo.label,
> label_len16);
> > 927 label = malloc(label_len + 1);
> > 928 if (!label) {
> CID 303760:(TAINTED_SCALAR)
> Passing tainted variable "data" to a tainted sink.
> > 929 free(data);
> > 930 ret = CMD_RET_FAILURE;
> > 931 goto out;
> > 932 }
> > 933 p = label;
> > 934 utf16_utf8_strncpy(, lo.label, label_len16);
>
> In CID 303760 the logic is as follows:
>
> In show_efi_boot_order() we malloc() memory. The allocated buffer is
> filled via byte swapping (get_unaligned_le16(), get_unaligned_le32()).
>
> Here comes Coverity's logic: "byte_swapping: Performing a byte swapping
> operation on ... implies that it came from an external source, and is
> therefore tainted."
>
> Now we pass the pointer to free(). Free() looks at 16 bytes preceding
> the pointer. Therefore free() is considered a tainted sink and an issue
> is raised.
>
> https://community.synopsys.com/s/article/From-Case-Clearing-TAINTED-STRING
> suggests to use Coverity specific comments to mark cleansing functions.
> This is not what I am inclined to do.
>
> CCing Takahiro as he had a hand in this code.
So, option B on that link is what I was thinking about which is creating
a function in the model file to tell Coverity it's handled. I was
going to include what ours was already as I thought I had written one,
but there's not one in the dashboard currently. And frankly a drawback
of Coverity is that you can't iterate on testing those kind of things
easily.
Option C is to just mark this (and the similar ones you can see via the
dashboard) as false positive.
--
Tom
signature.asc
Description: PGP signature
Re: Fwd: New Defects reported by Coverity Scan for Das U-Boot
On 26.05.20 20:40, Tom Rini wrote:
> Ah, I thought you might not have been part of Coverity.
> https://scan.coverity.com/projects/das-u-boot is where to start, it will
> take GitHub credentials and then I can approve you.
Thanks for granting access. In the GUI one can really drill down into
the explanation of the problem. This is very helpful.
> ** CID 303760:(TAINTED_SCALAR)
>
>
>
> *** CID 303760:(TAINTED_SCALAR)
> /cmd/efidebug.c: 938 in show_efi_boot_order()
> 932 }
> 933 p = label;
> 934 utf16_utf8_strncpy(, lo.label, label_len16);
> 935 printf("%2d: %s: %s\n", i + 1, var_name, label);
> 936 free(label);
> 937
CID 303760:(TAINTED_SCALAR)
Passing tainted variable "data" to a tainted sink.
> 938 free(data);
> 939 }
> 940 out:
> 941 free(bootorder);
> 942
> 943 return ret;
> /cmd/efidebug.c: 929 in show_efi_boot_order()
> 923 efi_deserialize_load_option(, data);
> 924
> 925 label_len16 = u16_strlen(lo.label);
> 926 label_len = utf16_utf8_strnlen(lo.label,
label_len16);
> 927 label = malloc(label_len + 1);
> 928 if (!label) {
CID 303760:(TAINTED_SCALAR)
Passing tainted variable "data" to a tainted sink.
> 929 free(data);
> 930 ret = CMD_RET_FAILURE;
> 931 goto out;
> 932 }
> 933 p = label;
> 934 utf16_utf8_strncpy(, lo.label, label_len16);
In CID 303760 the logic is as follows:
In show_efi_boot_order() we malloc() memory. The allocated buffer is
filled via byte swapping (get_unaligned_le16(), get_unaligned_le32()).
Here comes Coverity's logic: "byte_swapping: Performing a byte swapping
operation on ... implies that it came from an external source, and is
therefore tainted."
Now we pass the pointer to free(). Free() looks at 16 bytes preceding
the pointer. Therefore free() is considered a tainted sink and an issue
is raised.
https://community.synopsys.com/s/article/From-Case-Clearing-TAINTED-STRING
suggests to use Coverity specific comments to mark cleansing functions.
This is not what I am inclined to do.
CCing Takahiro as he had a hand in this code.
Best regards
Heinrich
