Re: [PATCH 1/2] RISC-V: KVM: Fix array out-of-bounds in pmu_ctr_read()

[Jiakai Xu]

Hi drew,

Thanks for the clarification.

> Any changes that come out of the pr_warn audit will result in a separate
> patch or patches. That work can be done completely separately and submit
> as a separate series. Or, if you do it right now, you could append those
> patches to this series. Either way works for me.

I will submit the pr_warn cleanup as a separate patch series later.

> That's good and we should do that, but we should also do negative testing.
> So there should be a test case where we try to read a counter without
> configuring it and ensure everything fails gracefully.

Agreed. In v2, I will update the selftests to include both positive tests 
and negative tests.

> > diff --git a/tools/testing/selftests/kvm/riscv/sbi_pmu_test.c b/tools/testing/selftests/kvm/riscv/sbi_pmu_test.c
> > index 924a335d2262..0d6ba3563561 100644
> > --- a/tools/testing/selftests/kvm/riscv/sbi_pmu_test.c
> > +++ b/tools/testing/selftests/kvm/riscv/sbi_pmu_test.c
> > @@ -461,7 +461,14 @@ static void test_pmu_basic_sanity(void)
> >  			pmu_csr_read_num(ctrinfo.csr);
> >  			GUEST_ASSERT(illegal_handler_invoked);
> >  		} else if (ctrinfo.type == SBI_PMU_CTR_TYPE_FW) {
> > -			read_fw_counter(i, ctrinfo);
> > +			/*
> > +			 * Try to configure with a common firmware event.
> > +			 * If configuration succeeds, verify we can read it.
> > +			 */
> > +			ret = sbi_ecall(SBI_EXT_PMU, SBI_EXT_PMU_COUNTER_CFG_MATCH,
> > +			        i, 1, 0, SBI_PMU_FW_ACCESS_LOAD, 0, 0);
> > +			if (ret.error == 0 && ret.value < RISCV_MAX_PMU_COUNTERS && BIT(ret.value) & counter_mask_available)
> 
> Put () around the & operator. checkpatch should have pointed that out.
> 

Noted, I will fix it in the next version.

> > +				read_fw_counter(i, ctrinfo);
> >  		}
> >  	}
> >  
> > -- 
> > 2.34.1
> >

I'll send out v2 shortly.

Thanks,
Jiakai