[PATCH 1/2] lib: sbi: Add the bound check for firmware events

[Andrew Jones]

On Mon, Jul 18, 2022 at 01:04:57PM -0700, Atish Patra wrote:
> Currently, there is no sanity check for firmware event code. We don't see
> any issue as Linux kernel driver does a bound check on firmware events
> already. However, OpenSBI can't assume sane supervisor mode software
> always. Thus, an invalid event idx can cause a buffer overflow error.
> 
> Improve the sanity check for firmware events.
> 
> Signed-off-by: Atish Patra <atishp at rivosinc.com>
> ---
>  lib/sbi/sbi_pmu.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/sbi/sbi_pmu.c b/lib/sbi/sbi_pmu.c
> index 3f5fd1031b25..65bc9dd63f6b 100644
> --- a/lib/sbi/sbi_pmu.c
> +++ b/lib/sbi/sbi_pmu.c
> @@ -123,6 +123,9 @@ static int pmu_ctr_validate(uint32_t cidx, uint32_t *event_idx_code)
>  		return SBI_EINVAL;
>  
>  	*event_idx_code = get_cidx_code(event_idx_val);
> +	if ((event_idx_type == SBI_PMU_EVENT_TYPE_FW) &&
> +	    (*event_idx_code >= SBI_PMU_FW_MAX))
> +		return SBI_EINVAL;

I think it would be better to only write *event_idx_code if we know
it's valid. Actually, maybe at the start of pmu_ctr_validate() we
should initialize it to something we know is invalid and then at the
bottom write it after we're sure we have something worth writing.

If pmu_ctr_validate() should be used to validate all event types, then
shouldn't we have a switch here, something like...

  switch (event_idx_type) {
  case SBI_PMU_EVENT_TYPE_HW:
    if (code >= SBI_PMU_HW_MAX)
        return SBI_EINVAL;
    break;
  case SBI_PMU_EVENT_TYPE_HW_CACHE:
    if (code >= SBI_PMU_HW_CACHE_MAX)
        return SBI_EINVAL;
    break;
  ...

>  
>  	return event_idx_type;
>  }
> @@ -652,6 +655,8 @@ skip_match:
>  			pmu_ctr_start_hw(ctr_idx, 0, false);
>  	} else if (event_type == SBI_PMU_EVENT_TYPE_FW) {
>  		fw_evt_code = get_cidx_code(event_idx);
> +		if (unlikely(fw_evt_code >= SBI_PMU_FW_MAX))
> +			return SBI_ENOTSUPP;

Validation buried down here is a bit scary. It would be much nicer if we
could pull all the validation up to the ecall entry points.

>  		fevent = &fw_event_map[hartid][fw_evt_code];
>  		if (flags & SBI_PMU_CFG_FLAG_CLEAR_VALUE)
>  			fevent->curr_count = 0;
> @@ -721,7 +726,7 @@ static void pmu_reset_event_map(u32 hartid)
>  	/* Initialize the counter to event mapping table */
>  	for (j = 3; j < total_ctrs; j++)
>  		active_events[hartid][j] = SBI_PMU_EVENT_IDX_INVALID;
> -	for (j = 0; j < SBI_PMU_FW_CTR_MAX; j++)
> +	for (j = 0; j < SBI_PMU_FW_EVENT_MAX; j++)

This change is not related to what the commit message is talking about,
so it belongs in a separate patch.

Thanks,
drew

>  		sbi_memset(&fw_event_map[hartid][j], 0,
>  			   sizeof(struct sbi_pmu_fw_event));
>  }
> -- 
> 2.25.1
> 
> 
> -- 
> opensbi mailing list
> opensbi at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/opensbi