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

[Andrew Jones]

Hi Atish,

Top posting because there's no cover letter to reply to. While reviewing
for this patch I see another buffer overrun possibility in
pmu_ctr_validate()

static int pmu_ctr_validate(uint32_t cidx, uint32_t *event_idx_code)
{
        uint32_t event_idx_val;
        uint32_t event_idx_type;
        u32 hartid = current_hartid();

        event_idx_val = active_events[hartid][cidx]; <--- Here, because...

...we have...

int sbi_pmu_ctr_read(uint32_t cidx, unsigned long *cval)
{
        int event_idx_type;
        uint32_t event_code;
        uint64_t cval64;

        event_idx_type = pmu_ctr_validate(cidx, &event_code);

...where sbi_pmu_ctr_read() is called directly from the ecall handler
passing a0 for cidx.

Thanks,
drew


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;
>  
>  	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;
>  		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++)
>  		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