> On 1 Sep 2016, at 07:50 AM, P J P <ppan...@redhat.com> wrote:
>
> Hello Dmitry,
>
> +-- On Wed, 31 Aug 2016, Dmitry Fleytman wrote --+
> | > - if ((ri->reqRingNumPages > PVSCSI_SETUP_RINGS_MAX_NUM_PAGES)
> | > - || (ri->cmpRingNumPages > PVSCSI_SETUP_RINGS_MAX_NUM_PAGES)) {
> | > - return -1;
> | > - }
> |
> | Hello Prasad,
> |
> | Why did you decide to move this logic out of pvscsi_ring_init_data()?
> | Why not just amend existing “if" as you did in v1 of this patch?
>
> 'ri->reqRingNumPages' and 'ri->cmpRingNumPages' values are also used in
> routine 'pvscsi_dbg_dump_tx_rings_config' before 'pvscsi_ring_init_data'
> call.
> if they were to have arbitrary values, this loop could run longer leading to
> OOB memory access.
>
> for (i = 0; i < rc->reqRingNumPages; i++) {
>
> trace_pvscsi_tx_rings_ppn("Request Ring", rc->reqRingPPNs[i]);
>
> }
>
> Moving above logic to 'pvscsi_on_cmd_setup_rings' helps both functions.
I see. Thanks.
In this case, please change pvscsi_ring_init_data() return value type to void.
Also I would suggest to do the new verification after
"trace_pvscsi_on_cmd_arrived("PVSCSI_CMD_SETUP_RINGS”)”.
>
> Thank you.
> --
> Prasad J Pandit / Red Hat Product Security Team
> 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
