[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 3/3] scsi: megasas: check 'read_queue_head' inde
From: |
Hannes Reinecke |
Subject: |
Re: [Qemu-devel] [PATCH 3/3] scsi: megasas: check 'read_queue_head' index value |
Date: |
Wed, 25 May 2016 15:21:21 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.7.0 |
On 05/25/2016 01:20 PM, Alexander Graf wrote:
> On 05/25/2016 12:31 PM, P J P wrote:
>> From: Prasad J Pandit <address@hidden>
>>
>> While doing MegaRAID SAS controller command frame lookup, routine
>> 'megasas_lookup_frame' uses 'read_queue_head' value as an index
>> into 'frames[MEGASAS_MAX_FRAMES=2048]' array. Limit its value
>> within array bounds to avoid any OOB access.
>>
>> Reported-by: Li Qiang <address@hidden>
>> Signed-off-by: Prasad J Pandit <address@hidden>
>> ---
>> hw/scsi/megasas.c | 6 ++++--
>> 1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
>> index 7c08932..76d57f9 100644
>> --- a/hw/scsi/megasas.c
>> +++ b/hw/scsi/megasas.c
>> @@ -649,8 +649,10 @@ static int megasas_init_firmware(MegasasState
>> *s, MegasasCmd *cmd)
>> pa_lo = le32_to_cpu(initq->pi_addr_lo);
>> pa_hi = le32_to_cpu(initq->pi_addr_hi);
>> s->producer_pa = ((uint64_t) pa_hi << 32) | pa_lo;
>> - s->reply_queue_head = ldl_le_pci_dma(pcid, s->producer_pa);
>> - s->reply_queue_tail = ldl_le_pci_dma(pcid, s->consumer_pa);
>> + s->reply_queue_head = \
>> + ldl_le_pci_dma(pcid, s->producer_pa) % MEGASAS_MAX_FRAMES;
>> + s->reply_queue_tail = \
>> + ldl_le_pci_dma(pcid, s->consumer_pa) % MEGASAS_MAX_FRAMES;
>
> You're using up 2 lines for each of those assignments now anyway, so
> why not just split it into
>
> s->reply_queue_head = ldl_le_pci_dma(pcid, s->producer_pa);
> s->reply_queue_head %= MEGASAS_MAX_FRAMES;
>
> and that way avoid a real multi-line statement (which is hard to read).
>
> Whether the bitmask really is what hardware would do, I don't know.
> I'll let Hannes comment there. Maybe the hardware simply does
>
> s->reply_queue_head = MIN(ldl_le_pci_dma(pcid, s->producer_pa),
> MEGASAS_MAX_FRAMES);
>
> But I agree that the mask is more likely.
>
->reply_queue_head (and, consequently, ->producer_pa) is a ring
buffer, which are supposed to wrap at MEGASAS_MAX_FRAMES.
So 'MIN' is wrong; we need the mask as we _want_ the overflow to happen.
Cheers,
Hannes
--
Dr. Hannes Reinecke Teamlead Storage & Networking
address@hidden +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
- [Qemu-devel] [PATCH 1/3] scsi: megasas: use appropriate property buffer size, (continued)
[Qemu-devel] [PATCH 2/3] scsi: megasas: initialise local configuration data buffer, P J P, 2016/05/25
[Qemu-devel] [PATCH 3/3] scsi: megasas: check 'read_queue_head' index value, P J P, 2016/05/25
Re: [Qemu-devel] [PATCH 0/3] Qemu: scsi: megasas: various OOB r/w access checks, Paolo Bonzini, 2016/05/25