[M] Change in coreboot[main]: tree: Use memcpy_s instead of memcpy

[Angel Pons (Code Review)]

Attention is currently required from: Alexander Couzens, Elyes Haouas, Erik van den Bogaert, Frans Hendriks, Intel coreboot Reviewers, Jeremy Soller, Johnny Lin, Matt DeVillier, Michael Niewöhner, Michał Żygowski, Morgan Jang, Piotr Król, Sean Rhodes, Tim Crawford.

Angel Pons has posted comments on this change by Elyes Haouas. ( https://review.coreboot.org/c/coreboot/+/86234?usp=email )

Change subject: tree: Use memcpy_s instead of memcpy
......................................................................

Patch Set 4: Code-Review-1

(52 comments)

Commit Message:

https://review.coreboot.org/c/coreboot/+/86234/comment/2aa79ca6_6a74a6d4?usp... :
PS4, Line 7: tree: Use memcpy_s instead of memcpy
Why? `memcpy_s` is not inherently more secure. Most of the changes here would actually break stuff, btw.

File src/drivers/i2c/at24rf08c/lenovo_serials.c:

https://review.coreboot.org/c/coreboot/+/86234/comment/57107c79_91599cde?usp... :
PS4, Line 49: sizeof(result)
`sizeof()` returns pointer size, which is not destination size. This cannot work.

https://review.coreboot.org/c/coreboot/+/86234/comment/cfaac87b_eb23c585?usp... :
PS4, Line 64: sizeof(result)
`sizeof()` returns pointer size, which is not destination size. This cannot work.

https://review.coreboot.org/c/coreboot/+/86234/comment/8cb3bfbc_267f16d4?usp... :
PS4, Line 169: sizeof(result)
Instead of that, you could `_Static_Assert(sizeof(result) >= sizeof(ERROR_STRING))`

File src/drivers/ipmi/ipmi_kcs_ops.c:

https://review.coreboot.org/c/coreboot/+/86234/comment/96496ac0_1b03790a?usp... :
PS4, Line 137: 	memcpy_s(&man_id, sizeof(&man_id), rsp.manufacturer_id, sizeof(rsp.manufacturer_id));
             : 
             : 	memcpy_s(&prod_id, sizeof(&prod_id), rsp.product_id, sizeof(rsp.product_id));
I have no idea why these use memcpy. Is `rsp` packed, so the fields are misaligned?

File src/mainboard/51nb/x210/romstage.c:

https://review.coreboot.org/c/coreboot/+/86234/comment/ba05bcdf_1b3a4e8b?usp... :
PS4, Line 12: sizeof(rcomp_ptr)
`sizeof()` returns pointer size, which is not destination size. This cannot work.

https://review.coreboot.org/c/coreboot/+/86234/comment/29fd7df7_81d7ffaa?usp... :
PS4, Line 19: sizeof(rcomp_strength_ptr)
`sizeof()` returns pointer size, which is not destination size. This cannot work.

File src/mainboard/clevo/kbl-u/variants/n13xwu/romstage.c:

https://review.coreboot.org/c/coreboot/+/86234/comment/39b2cc25_308d96d5?usp... :
PS4, Line 11: sizeof(rcomp_ptr)
`sizeof()` returns pointer size, which is not destination size. This cannot work.

https://review.coreboot.org/c/coreboot/+/86234/comment/0029eb8a_56425af0?usp... :
PS4, Line 17: sizeof(rcomp_strength_ptr)
`sizeof()` returns pointer size, which is not destination size. This cannot work.

File src/mainboard/facebook/monolith/spd/spd_util.c:

https://review.coreboot.org/c/coreboot/+/86234/comment/c2c4cb03_be0af6f5?usp... :
PS4, Line 16: sizeof(dq_map_ptr)
`sizeof()` returns pointer size, which is not destination size. This cannot work.

https://review.coreboot.org/c/coreboot/+/86234/comment/3386fb6e_60cb7485?usp... :
PS4, Line 25: sizeof(dqs_map_ptr)
`sizeof()` returns pointer size, which is not destination size. This cannot work.

https://review.coreboot.org/c/coreboot/+/86234/comment/044881b1_a8781088?usp... :
PS4, Line 41: sizeof(rcomp_strength_ptr)
`sizeof()` returns pointer size, which is not destination size. This cannot work.

File src/mainboard/getac/p470/acpi_tables.c:

https://review.coreboot.org/c/coreboot/+/86234/comment/1c2de7df_bfc2b75a?usp... :
PS4, Line 48: sizeof(ecdt->ec_id)
`ec_id` is a zero-length array, `sizeof()` will not return a meaningful value. Note that the string size is already accounted for in the code.

File src/mainboard/google/eve/romstage.c:

https://review.coreboot.org/c/coreboot/+/86234/comment/4782e007_5d12e02c?usp... :
PS4, Line 32: memcpy_s(&mem_cfg->DqByteMapCh0, sizeof(&mem_cfg->DqByteMapCh0), dq_map[0], sizeof(dq_map[0]));
            : 	memcpy_s(&mem_cfg->DqByteMapCh1, sizeof(&mem_cfg->DqByteMapCh1), dq_map[1], sizeof(dq_map[1]));
            : 	memcpy_s(&mem_cfg->DqsMapCpu2DramCh0, sizeof(&mem_cfg->DqsMapCpu2DramCh0), dqs_map[0], sizeof(dqs_map[0]));
            : 	memcpy_s(&mem_cfg->DqsMapCpu2DramCh1, sizeof(&mem_cfg->DqsMapCpu2DramCh1), dqs_map[1], sizeof(dqs_map[1]));
            : 	memcpy_s(&mem_cfg->RcompResistor, sizeof(&mem_cfg->RcompResistor), rcomp_resistor, sizeof(rcomp_resistor));
            : 	memcpy_s(&mem_cfg->RcompTarget, sizeof(&mem_cfg->RcompTarget), rcomp_target, sizeof(rcomp_target));
It would be better to adapt the API so that boards don't have to `memcpy` these arrays. Then one can just have one copy of a `_Static_Assert()` in SoC code in case FSP UPDs change (which is unlikely).

Moreover, Rcomp settings aren't always mainboard-specific. SoC code could provide sane defaults, and only override the settings from mainboards that need special values.

File src/mainboard/intel/kblrvp/spd/spd_util.c:

https://review.coreboot.org/c/coreboot/+/86234/comment/99cccd59_72d769dd?usp... :
PS4, Line 17: sizeof(dq_map_ch0)
`sizeof()` returns pointer size, which is not destination size. This cannot work.

https://review.coreboot.org/c/coreboot/+/86234/comment/ceba72b4_e5763295?usp... :
PS4, Line 18: sizeof(dq_map_ch1)
`sizeof()` returns pointer size, which is not destination size. This cannot work.

https://review.coreboot.org/c/coreboot/+/86234/comment/4a5956ad_57301d99?usp... :
PS4, Line 27: sizeof(dqs_map_ch0)
`sizeof()` returns pointer size, which is not destination size. This cannot work.

https://review.coreboot.org/c/coreboot/+/86234/comment/ac857987_66056ba1?usp... :
PS4, Line 28: sizeof(dqs_map_ch1)
`sizeof()` returns pointer size, which is not destination size. This cannot work.

https://review.coreboot.org/c/coreboot/+/86234/comment/5ebc287b_961f0155?usp... :
PS4, Line 44: sizeof(rcomp_strength_ptr)
`sizeof()` returns pointer size, which is not destination size. This cannot work.

File src/mainboard/intel/kunimitsu/spd/spd_util.c:

https://review.coreboot.org/c/coreboot/+/86234/comment/da02e2b1_a71f8332?usp... :
PS4, Line 20: sizeof(dq_map_ch0)
`sizeof()` returns pointer size, which is not destination size. This cannot work.

https://review.coreboot.org/c/coreboot/+/86234/comment/fee9bc13_be4a19ff?usp... :
PS4, Line 21: sizeof(dq_map_ch1)
`sizeof()` returns pointer size, which is not destination size. This cannot work.

https://review.coreboot.org/c/coreboot/+/86234/comment/13223e1f_d895ec7e?usp... :
PS4, Line 30: sizeof(dqs_map_ch0)
`sizeof()` returns pointer size, which is not destination size. This cannot work.

https://review.coreboot.org/c/coreboot/+/86234/comment/28ec8740_2c594500?usp... :
PS4, Line 31: sizeof(dqs_map_ch1)
`sizeof()` returns pointer size, which is not destination size. This cannot work.

https://review.coreboot.org/c/coreboot/+/86234/comment/f311bdf5_a4e52cab?usp... :
PS4, Line 59: sizeof(rcomp_strength_ptr)
`sizeof()` returns pointer size, which is not destination size. This cannot work.

File src/mainboard/intel/saddlebrook/spd/spd_util.c:

https://review.coreboot.org/c/coreboot/+/86234/comment/47b856c2_4ce807ea?usp... :
PS4, Line 15: sizeof(dq_map_ch0)
`sizeof()` returns pointer size, which is not destination size. This cannot work.

https://review.coreboot.org/c/coreboot/+/86234/comment/a0fa9e73_87069a97?usp... :
PS4, Line 16: sizeof(dq_map_ch1)
`sizeof()` returns pointer size, which is not destination size. This cannot work.

https://review.coreboot.org/c/coreboot/+/86234/comment/bd9f62c1_809041e3?usp... :
PS4, Line 25: sizeof(dqs_map_ch0)
`sizeof()` returns pointer size, which is not destination size. This cannot work.

https://review.coreboot.org/c/coreboot/+/86234/comment/cd0b86cd_733b5faf?usp... :
PS4, Line 26: sizeof(dqs_map_ch1)
`sizeof()` returns pointer size, which is not destination size. This cannot work.

https://review.coreboot.org/c/coreboot/+/86234/comment/e597791e_c915fc38?usp... :
PS4, Line 33: sizeof(rcomp_ptr)
`sizeof()` returns pointer size, which is not destination size. This cannot work.

https://review.coreboot.org/c/coreboot/+/86234/comment/c1b16584_c742cfe3?usp... :
PS4, Line 40: sizeof(rcomp_strength_ptr)
`sizeof()` returns pointer size, which is not destination size. This cannot work.

File src/mainboard/lenovo/thinkcentre_m710s/romstage.c:

https://review.coreboot.org/c/coreboot/+/86234/comment/83c7ab5a_83f04ce9?usp... :
PS4, Line 31: 	assert(sizeof(mem_cfg->RcompResistor) == sizeof(rcomp_resistors));
There's this assertion already.

https://review.coreboot.org/c/coreboot/+/86234/comment/917e05db_054c16d5?usp... :
PS4, Line 36: 	assert(sizeof(mem_cfg->RcompTarget) == sizeof(rcomp_targets));
There's this assertion already.

File src/mainboard/libretrend/lt1000/romstage.c:

https://review.coreboot.org/c/coreboot/+/86234/comment/2aa9ed2a_0b486d5e?usp... :
PS4, Line 14: sizeof(dq_map_ch0)
`sizeof()` returns pointer size, which is not destination size. This cannot work.

https://review.coreboot.org/c/coreboot/+/86234/comment/6d0ec0d6_006cebd5?usp... :
PS4, Line 15: sizeof(dq_map_ch1)
`sizeof()` returns pointer size, which is not destination size. This cannot work.

https://review.coreboot.org/c/coreboot/+/86234/comment/cff16a02_a200c7a4?usp... :
PS4, Line 23: sizeof(dqs_map_ch0)
`sizeof()` returns pointer size, which is not destination size. This cannot work.

https://review.coreboot.org/c/coreboot/+/86234/comment/72f3afdf_f91fbdd3?usp... :
PS4, Line 24: sizeof(dqs_map_ch1)
`sizeof()` returns pointer size, which is not destination size. This cannot work.

https://review.coreboot.org/c/coreboot/+/86234/comment/2c886e70_9fa5f5a0?usp... :
PS4, Line 30: sizeof(rcomp_ptr)
`sizeof()` returns pointer size, which is not destination size. This cannot work.

https://review.coreboot.org/c/coreboot/+/86234/comment/18d405ba_c398b876?usp... :
PS4, Line 36: sizeof(rcomp_strength_ptr)
`sizeof()` returns pointer size, which is not destination size. This cannot work.

File src/mainboard/ocp/tiogapass/romstage.c:

https://review.coreboot.org/c/coreboot/+/86234/comment/14f236bd_5170094e?usp... :
PS4, Line 39: sizeof(iio_table_buf)
Look at how `tp_iio_bifur_table` is defined. It is guaranteed to be of the same size, so no need for a runtime check.

File src/mainboard/protectli/vault_kbl/romstage.c:

https://review.coreboot.org/c/coreboot/+/86234/comment/403f54d0_82e3b01a?usp... :
PS4, Line 15: sizeof(dq_map_ch0)
`sizeof()` returns pointer size, which is not destination size. This cannot work.

https://review.coreboot.org/c/coreboot/+/86234/comment/a71ea974_23269087?usp... :
PS4, Line 16: sizeof(dq_map_ch1)
`sizeof()` returns pointer size, which is not destination size. This cannot work.

https://review.coreboot.org/c/coreboot/+/86234/comment/75d774db_933f9241?usp... :
PS4, Line 24: sizeof(dqs_map_ch0)
`sizeof()` returns pointer size, which is not destination size. This cannot work.

https://review.coreboot.org/c/coreboot/+/86234/comment/dd6b1d42_7d6ccf57?usp... :
PS4, Line 25: sizeof(dqs_map_ch1)
`sizeof()` returns pointer size, which is not destination size. This cannot work.

https://review.coreboot.org/c/coreboot/+/86234/comment/c633caea_3e74b1ce?usp... :
PS4, Line 31: sizeof(rcomp_ptr)
`sizeof()` returns pointer size, which is not destination size. This cannot work.

https://review.coreboot.org/c/coreboot/+/86234/comment/2a3c9ed5_982c5404?usp... :
PS4, Line 37: sizeof(rcomp_strength_ptr)
`sizeof()` returns pointer size, which is not destination size. This cannot work.

File src/mainboard/razer/blade_stealth_kbl/spd/spd_util.c:

https://review.coreboot.org/c/coreboot/+/86234/comment/c23e8e0b_8e2d41e7?usp... :
PS4, Line 14: sizeof(dq_map_ptr)
`sizeof()` returns pointer size, which is not destination size. This cannot work.

https://review.coreboot.org/c/coreboot/+/86234/comment/64aaf902_14426b95?usp... :
PS4, Line 21: sizeof(dqs_map_ptr)
`sizeof()` returns pointer size, which is not destination size. This cannot work.

https://review.coreboot.org/c/coreboot/+/86234/comment/f09cbf65_ebd428b3?usp... :
PS4, Line 28: sizeof(rcomp_ptr)
`sizeof()` returns pointer size, which is not destination size. This cannot work.

https://review.coreboot.org/c/coreboot/+/86234/comment/75446cf4_8534349d?usp... :
PS4, Line 36: sizeof(rcomp_strength_ptr)
`sizeof()` returns pointer size, which is not destination size. This cannot work.

File src/mainboard/system76/kbl-u/romstage.c:

https://review.coreboot.org/c/coreboot/+/86234/comment/9dee6c81_c5679ac3?usp... :
PS4, Line 10: sizeof(rcomp_ptr)
`sizeof()` returns pointer size, which is not destination size. This cannot work.

https://review.coreboot.org/c/coreboot/+/86234/comment/9206bef9_a54d7574?usp... :
PS4, Line 16: sizeof(rcomp_strength_ptr)
`sizeof()` returns pointer size, which is not destination size. This cannot work.

File src/northbridge/intel/x4x/dq_dqs.c:

https://review.coreboot.org/c/coreboot/+/86234/comment/09403b7c_e797e1c7?usp... :
PS4, Line 280: sizeof(dq_upper)
Both arrays are of the same size.