ocp-smart-extended-log: fix sizeof mismatch in malloc for get_c0_log_page()

[sahmed-ibm]

The get_c0_log_page() function allocates a buffer @DaTa to hold the C0 SMART extended log page, but passes sizeof of an unrelated scalar type as the malloc operand instead of sizeof of the struct @DaTa points to.

If the compiler inserts alignment padding into the struct, the allocation will be smaller than the struct size, causing a heap buffer overflow when the struct fields are accessed beyond the allocated region.

Fix by using sizeof(*data) to ensure the correct amount of memory is allocated and zeroed for the struct.

[igaw]

according pahole

› pahole ./.build/nvme -C ocp_smart_extended_log
struct ocp_smart_extended_log {
	__u8                       physical_media_units_written[16]; /*     0    16 */
	__u8                       physical_media_units_read[16]; /*    16    16 */
	__u8                       bad_user_nand_blocks_raw[6]; /*    32     6 */
	__le16                     bad_user_nand_blocks_normalized; /*    38     2 */
	__u8                       bad_system_nand_blocks_raw[6]; /*    40     6 */
	__le16                     bad_system_nand_blocks_normalized; /*    46     2 */
	__le64                     xor_recovery_count;   /*    48     8 */
	__le64                     uncorrectable_read_err_count; /*    56     8 */
	/* --- cacheline 1 boundary (64 bytes) --- */
	__le64                     soft_ecc_err_count;   /*    64     8 */
	__le32                     end_to_end_detected_err; /*    72     4 */
	__le32                     end_to_end_corrected_err; /*    76     4 */
	__u8                       system_data_used_percent; /*    80     1 */
	__u8                       refresh_counts[7];    /*    81     7 */
	__le32                     user_data_erase_count_max; /*    88     4 */
	__le32                     user_data_erase_count_min; /*    92     4 */
	__u8                       thermal_throttling_event_count; /*    96     1 */
	__u8                       thermal_throttling_current_status; /*    97     1 */
	__u8                       dssd_errata_version;  /*    98     1 */
	__u8                       dssd_point_version[2]; /*    99     2 */
	__u8                       dssd_minor_version[2]; /*   101     2 */
	__u8                       dssd_major_version;   /*   103     1 */
	__le64                     pcie_correctable_err_count; /*   104     8 */
	__le32                     incomplete_shoutdowns; /*   112     4 */
	__u8                       rsvd116[4];           /*   116     4 */
	__u8                       percent_free_blocks;  /*   120     1 */
	__u8                       rsvd121[7];           /*   121     7 */
	/* --- cacheline 2 boundary (128 bytes) --- */
	__le16                     capacitor_health;     /*   128     2 */
	__u8                       nvme_base_errata_version; /*   130     1 */
	__u8                       nvme_cmdset_errata_version; /*   131     1 */
	__u8                       nvme_over_pcie_errate_version; /*   132     1 */
	__u8                       nvme_mi_errata_version; /*   133     1 */
	__u8                       rsvd134[2];           /*   134     2 */
	__le64                     unaligned_io;         /*   136     8 */
	__le64                     security_version;     /*   144     8 */
	__le64                     total_nuse;           /*   152     8 */
	__u8                       plp_start_count[16];  /*   160    16 */
	__u8                       endurance_estimate[16]; /*   176    16 */
	/* --- cacheline 3 boundary (192 bytes) --- */
	__le64                     pcie_link_retaining_count; /*   192     8 */
	__le64                     power_state_change_count; /*   200     8 */
	__le64                     lowest_permitted_fw_rev; /*   208     8 */
	__le16                     total_media_dies;     /*   216     2 */
	__le16                     total_die_failure_tolerance; /*   218     2 */
	__le16                     media_dies_offline;   /*   220     2 */
	__u8                       max_temperature_recorded; /*   222     1 */
	__u8                       form_factor;          /*   223     1 */
	__le64                     nand_avg_erase_count; /*   224     8 */
	__le32                     command_timeouts;     /*   232     4 */
	__le32                     sys_area_program_fail_count_raw; /*   236     4 */
	__u8                       sys_area_program_fail_count_normalized; /*   240     1 */
	__u8                       rsvd241[3];           /*   241     3 */
	__le32                     sys_area_uncorr_read_count_raw; /*   244     4 */
	__u8                       sys_area_uncorr_read_count_normalized; /*   248     1 */
	__u8                       rsvd249[3];           /*   249     3 */
	__le32                     sys_area_erase_fail_count_raw; /*   252     4 */
	/* --- cacheline 4 boundary (256 bytes) --- */
	__u8                       sys_area_erase_fail_count_normalized; /*   256     1 */
	__u8                       rsvd257[3];           /*   257     3 */
	__le16                     max_peak_power_capability; /*   260     2 */
	__le16                     current_max_avg_power; /*   262     2 */
	__u8                       lifetime_power_consumed[6]; /*   264     6 */
	__u8                       dssd_firmware_revision[8]; /*   270     8 */
	__u8                       dssd_firmware_build_uuid[16]; /*   278    16 */
	__u8                       dssd_firmware_build_label[64]; /*   294    64 */
	/* --- cacheline 5 boundary (320 bytes) was 38 bytes ago --- */
	__u8                       die_in_use_bad_nand_block_raw[6]; /*   358     6 */
	__le16                     die_in_use_bad_nand_block_normalized; /*   364     2 */
	__u8                       rsvd358[128];         /*   366   128 */
	/* --- cacheline 7 boundary (448 bytes) was 46 bytes ago --- */
	__le16                     log_page_version;     /*   494     2 */
	__u8                       log_page_guid[16];    /*   496    16 */

	/* size: 512, cachelines: 8, members: 67 */
};

which is in hex 0x200. This matches

 #define C0_SMART_CLOUD_ATTR_LEN                 0x200     

and later in the code we do

	nvme_init_get_log(&cmd, NVME_NSID_ALL,
			  (enum nvme_cmd_get_log_lid)OCP_LID_SMART,
			  NVME_CSI_NVM, data, C0_SMART_CLOUD_ATTR_LEN);

As far I understand your argument is that the compiler might insert padding? If this would happen the data struct would be useless.

[sahmed-ibm]

I see, I didn't realize the struct is exactly 512 bytes. That said, I'd still like to propose a small cleanup: the sizeof(__u8) * multiplier is redundant noise since sizeof(__u8) is always 1. Would you accept replacing it with just malloc(C0_SMART_CLOUD_ATTR_LEN) and memset(data, 0, C0_SMART_CLOUD_ATTR_LEN)?

[igaw]

I am more in the camp of using sizeof(*ptr) instead of the explicit length, which works well for non array cases.

With this in mind, what about making nvme_init_get_log also using sizeof and get rid of C0_SMART_CLOUD_ATTR_LEN which then is unused?

	data = malloc(sizeof(*data)); 

 	[...]

	nvme_init_get_log(&cmd, NVME_NSID_ALL,
			  (enum nvme_cmd_get_log_lid)OCP_LID_SMART,
			  NVME_CSI_NVM, data, sizeof(*data));

[sahmed-ibm]

Sure, that sounds good. I'll implement that!

[igaw]

Excellent work! Thanks!