Re: [PATCH 17/37] scsi: mpt3sas: Clamp ValidEntries in driver trigger page update helpers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] scsi: mpt3sas: signed count bypasses clamp in trigger update helpers
--

commit ae22d6dc5a3a1081272d193a5d2d62df9244f533
Author: Shivasharan S <shivasharan.srikanteshwara@broadcom.com>

scsi: mpt3sas: Clamp ValidEntries in driver trigger page update helpers

This commit clamps ValidEntries to NUM_VALID_ENTRIES in the pg2, pg3, and
pg4 driver trigger update helpers before using it as a loop bound and trigger
count.

> diff --git a/drivers/scsi/mpt3sas/mpt3sas_config.c b/drivers/scsi/mpt3sas/mpt3sas_config.c
> index 75312d43e3862..ddbccbac69b34 100644
> --- a/drivers/scsi/mpt3sas/mpt3sas_config.c
> +++ b/drivers/scsi/mpt3sas/mpt3sas_config.c
> @@ -2278,6 +2278,8 @@ mpt3sas_config_update_driver_trigger_pg2(struct MPT3SAS_ADAPTER *ioc,
>  
>  	if (set) {
>  		count = event_tg->ValidEntries;
> +		if (count > NUM_VALID_ENTRIES)
> +			count = NUM_VALID_ENTRIES;
>  		tg_pg2.NumMPIEventTrigger = cpu_to_le16(count);
>  		for (i = 0; i < count; i++) {
>  			tg_pg2.MPIEventTriggers[i].MPIEventCode =
> @@ -2437,6 +2439,8 @@ mpt3sas_config_update_driver_trigger_pg3(struct MPT3SAS_ADAPTER *ioc,
>  
>  	if (set) {
>  		count = scsi_tg->ValidEntries;
> +		if (count > NUM_VALID_ENTRIES)
> +			count = NUM_VALID_ENTRIES;
>  		tg_pg3.NumSCSISenseTrigger = cpu_to_le16(count);
>  		for (i = 0; i < count; i++) {
>  			tg_pg3.SCSISenseTriggers[i].ASCQ =
> @@ -2595,6 +2599,8 @@ mpt3sas_config_update_driver_trigger_pg4(struct MPT3SAS_ADAPTER *ioc,
>  
>  	if (set) {
>  		count = mpi_tg->ValidEntries;
> +		if (count > NUM_VALID_ENTRIES)
> +			count = NUM_VALID_ENTRIES;
>  		tg_pg4.NumIOCStatusLogInfoTrigger = cpu_to_le16(count);

[Severity: Medium]
Can a ValidEntries value with bit 31 set (>= 0x80000000) bypass this clamp in
mpt3sas_config_update_driver_trigger_pg2(),
mpt3sas_config_update_driver_trigger_pg3(), and
mpt3sas_config_update_driver_trigger_pg4()?

In drivers/scsi/mpt3sas/mpt3sas_trigger_diag.h, ValidEntries is declared as
uint32_t in struct SL_WH_EVENT_TRIGGERS_T, struct SL_WH_SCSI_TRIGGERS_T, and
struct SL_WH_MPI_TRIGGERS_T, whereas count in these three helpers in
drivers/scsi/mpt3sas/mpt3sas_config.c is a signed int:

	int rc, i, count;

The sysfs store handlers diag_trigger_event_store(),
diag_trigger_scsi_store(), and diag_trigger_mpi_store() in
drivers/scsi/mpt3sas/mpt3sas_ctl.c copy user data into event_tg, scsi_tg, or
mpi_tg and call these update helpers before clamping ValidEntries:

	memcpy(event_tg, buf, sz);
	if (!event_tg->ValidEntries)
		set = 0;
	if (mpt3sas_config_update_driver_trigger_pg2(ioc, event_tg,
	    set)) {

When a user passes ValidEntries >= 0x80000000, assigning it to the signed int
count makes count negative. The signed comparison count > NUM_VALID_ENTRIES
then evaluates to false, the for (i = 0; i < count; i++) loop copies zero
entries, and cpu_to_le16(count) writes the truncated unclamped u16 value into
tg_pg2.NumMPIEventTrigger, tg_pg3.NumSCSISenseTrigger, or
tg_pg4.NumIOCStatusLogInfoTrigger before committing the page to the
controller.

Should count (and i) be unsigned, or should event_tg->ValidEntries,
scsi_tg->ValidEntries, and mpi_tg->ValidEntries be clamped using min_t(u32,
..., NUM_VALID_ENTRIES) before assigning to count?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/1793925772-1303-1-git-send-email-shivasharan.srikanteshwara@broadcom.com?part=17