diff mbox

[v1,1/1] cadence_gem: Fix priority queue out of bounds access

Message ID 33bf2d28326d22875602234b8b15cf56fb678333.1474911607.git.alistair.francis@xilinx.com
State New
Headers show

Commit Message

Alistair Francis Sept. 26, 2016, 5:49 p.m. UTC 
There was an error with some of the register implementation assuming
there are 16 priority queues supported when the IP only supports 8. This
patch corrects the registers to only support 8 queues.

Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
Reported-by: Paolo Bonzini <pbonzini@redhat.com>
---
Thanks to Paolo for pointing this out.

 hw/net/cadence_gem.c | 22 ++++------------------
 1 file changed, 4 insertions(+), 18 deletions(-)

Comments

Peter Maydell Sept. 30, 2016, 12:04 a.m. UTC | #1 
On 26 September 2016 at 10:49, Alistair Francis
<alistair.francis@xilinx.com> wrote:
> There was an error with some of the register implementation assuming
> there are 16 priority queues supported when the IP only supports 8. This
> patch corrects the registers to only support 8 queues.
>
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> Reported-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> Thanks to Paolo for pointing this out.
>
>  hw/net/cadence_gem.c | 22 ++++------------------
>  1 file changed, 4 insertions(+), 18 deletions(-)
>
> diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
> index 8618e7a..7915732 100644
> --- a/hw/net/cadence_gem.c
> +++ b/hw/net/cadence_gem.c
> @@ -147,25 +147,19 @@
>  #define GEM_INT_Q1_MASK                 (0x00000640 / 4)
>
>  #define GEM_TRANSMIT_Q1_PTR             (0x00000440 / 4)
> -#define GEM_TRANSMIT_Q15_PTR            (GEM_TRANSMIT_Q1_PTR + 14)
> +#define GEM_TRANSMIT_Q7_PTR             (GEM_TRANSMIT_Q1_PTR + 6)

8 queues but they're numbered one to ... seven ??

thanks
-- PMM
Alistair Francis Sept. 30, 2016, 12:42 a.m. UTC | #2 
On Thu, Sep 29, 2016 at 5:04 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 26 September 2016 at 10:49, Alistair Francis
> <alistair.francis@xilinx.com> wrote:
>> There was an error with some of the register implementation assuming
>> there are 16 priority queues supported when the IP only supports 8. This
>> patch corrects the registers to only support 8 queues.
>>
>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>> Reported-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>> Thanks to Paolo for pointing this out.
>>
>>  hw/net/cadence_gem.c | 22 ++++------------------
>>  1 file changed, 4 insertions(+), 18 deletions(-)
>>
>> diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
>> index 8618e7a..7915732 100644
>> --- a/hw/net/cadence_gem.c
>> +++ b/hw/net/cadence_gem.c
>> @@ -147,25 +147,19 @@
>>  #define GEM_INT_Q1_MASK                 (0x00000640 / 4)
>>
>>  #define GEM_TRANSMIT_Q1_PTR             (0x00000440 / 4)
>> -#define GEM_TRANSMIT_Q15_PTR            (GEM_TRANSMIT_Q1_PTR + 14)
>> +#define GEM_TRANSMIT_Q7_PTR             (GEM_TRANSMIT_Q1_PTR + 6)
>
> 8 queues but they're numbered one to ... seven ??

Yeah, I double checked the user guide and it is 1 to 7.

I guess queue 0 uses the standard registers.

Thanks,

Alistair

>
> thanks
> -- PMM
>
Peter Maydell Sept. 30, 2016, 1:48 a.m. UTC | #3 
On 29 September 2016 at 17:42, Alistair Francis
<alistair.francis@xilinx.com> wrote:
> On Thu, Sep 29, 2016 at 5:04 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 26 September 2016 at 10:49, Alistair Francis
>> <alistair.francis@xilinx.com> wrote:
>>> There was an error with some of the register implementation assuming
>>> there are 16 priority queues supported when the IP only supports 8. This
>>> patch corrects the registers to only support 8 queues.
>>>
>>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>>> Reported-by: Paolo Bonzini <pbonzini@redhat.com>
>>> ---
>>> Thanks to Paolo for pointing this out.
>>>
>>>  hw/net/cadence_gem.c | 22 ++++------------------
>>>  1 file changed, 4 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
>>> index 8618e7a..7915732 100644
>>> --- a/hw/net/cadence_gem.c
>>> +++ b/hw/net/cadence_gem.c
>>> @@ -147,25 +147,19 @@
>>>  #define GEM_INT_Q1_MASK                 (0x00000640 / 4)
>>>
>>>  #define GEM_TRANSMIT_Q1_PTR             (0x00000440 / 4)
>>> -#define GEM_TRANSMIT_Q15_PTR            (GEM_TRANSMIT_Q1_PTR + 14)
>>> +#define GEM_TRANSMIT_Q7_PTR             (GEM_TRANSMIT_Q1_PTR + 6)
>>
>> 8 queues but they're numbered one to ... seven ??
>
> Yeah, I double checked the user guide and it is 1 to 7.
>
> I guess queue 0 uses the standard registers.

OK. The arithmetic of the array size vs the number of
registers looks OK, anyway.

Applied to target-arm.next.

thanks
-- PMM
diff mbox

Patch

diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
index 8618e7a..7915732 100644
--- a/hw/net/cadence_gem.c
+++ b/hw/net/cadence_gem.c
@@ -147,25 +147,19 @@ 
 #define GEM_INT_Q1_MASK                 (0x00000640 / 4)
 
 #define GEM_TRANSMIT_Q1_PTR             (0x00000440 / 4)
-#define GEM_TRANSMIT_Q15_PTR            (GEM_TRANSMIT_Q1_PTR + 14)
+#define GEM_TRANSMIT_Q7_PTR             (GEM_TRANSMIT_Q1_PTR + 6)
 
 #define GEM_RECEIVE_Q1_PTR              (0x00000480 / 4)
-#define GEM_RECEIVE_Q15_PTR             (GEM_RECEIVE_Q1_PTR + 14)
+#define GEM_RECEIVE_Q7_PTR              (GEM_RECEIVE_Q1_PTR + 6)
 
 #define GEM_INT_Q1_ENABLE               (0x00000600 / 4)
 #define GEM_INT_Q7_ENABLE               (GEM_INT_Q1_ENABLE + 6)
-#define GEM_INT_Q8_ENABLE               (0x00000660 / 4)
-#define GEM_INT_Q15_ENABLE              (GEM_INT_Q8_ENABLE + 7)
 
 #define GEM_INT_Q1_DISABLE              (0x00000620 / 4)
 #define GEM_INT_Q7_DISABLE              (GEM_INT_Q1_DISABLE + 6)
-#define GEM_INT_Q8_DISABLE              (0x00000680 / 4)
-#define GEM_INT_Q15_DISABLE             (GEM_INT_Q8_DISABLE + 7)
 
 #define GEM_INT_Q1_MASK                 (0x00000640 / 4)
 #define GEM_INT_Q7_MASK                 (GEM_INT_Q1_MASK + 6)
-#define GEM_INT_Q8_MASK                 (0x000006A0 / 4)
-#define GEM_INT_Q15_MASK                (GEM_INT_Q8_MASK + 7)
 
 #define GEM_SCREENING_TYPE1_REGISTER_0  (0x00000500 / 4)
 
@@ -1372,13 +1366,13 @@  static void gem_write(void *opaque, hwaddr offset, uint64_t val,
     case GEM_RXQBASE:
         s->rx_desc_addr[0] = val;
         break;
-    case GEM_RECEIVE_Q1_PTR ... GEM_RECEIVE_Q15_PTR:
+    case GEM_RECEIVE_Q1_PTR ... GEM_RECEIVE_Q7_PTR:
         s->rx_desc_addr[offset - GEM_RECEIVE_Q1_PTR + 1] = val;
         break;
     case GEM_TXQBASE:
         s->tx_desc_addr[0] = val;
         break;
-    case GEM_TRANSMIT_Q1_PTR ... GEM_TRANSMIT_Q15_PTR:
+    case GEM_TRANSMIT_Q1_PTR ... GEM_TRANSMIT_Q7_PTR:
         s->tx_desc_addr[offset - GEM_TRANSMIT_Q1_PTR + 1] = val;
         break;
     case GEM_RXSTATUS:
@@ -1392,10 +1386,6 @@  static void gem_write(void *opaque, hwaddr offset, uint64_t val,
         s->regs[GEM_INT_Q1_MASK + offset - GEM_INT_Q1_ENABLE] &= ~val;
         gem_update_int_status(s);
         break;
-    case GEM_INT_Q8_ENABLE ... GEM_INT_Q15_ENABLE:
-        s->regs[GEM_INT_Q8_MASK + offset - GEM_INT_Q8_ENABLE] &= ~val;
-        gem_update_int_status(s);
-        break;
     case GEM_IDR:
         s->regs[GEM_IMR] |= val;
         gem_update_int_status(s);
@@ -1404,10 +1394,6 @@  static void gem_write(void *opaque, hwaddr offset, uint64_t val,
         s->regs[GEM_INT_Q1_MASK + offset - GEM_INT_Q1_DISABLE] |= val;
         gem_update_int_status(s);
         break;
-    case GEM_INT_Q8_DISABLE ... GEM_INT_Q15_DISABLE:
-        s->regs[GEM_INT_Q8_MASK + offset - GEM_INT_Q8_DISABLE] |= val;
-        gem_update_int_status(s);
-        break;
     case GEM_SPADDR1LO:
     case GEM_SPADDR2LO:
     case GEM_SPADDR3LO: