Re: [Qemu-devel] [PATCH v2] scsi: check page count while initialising descriptor rings

[Dmitry Fleytman]

> On 1 Sep 2016, at 07:50 AM, P J P <address@hidden> 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