[v2] kvm: mpic: limit active IRQ sources to NUM_OUTPUTS
diff mbox series

Message ID 20191115050620.21360-1-ppandit@redhat.com
State Rejected
Headers show
Series
  • [v2] kvm: mpic: limit active IRQ sources to NUM_OUTPUTS
Related show

Commit Message

P J P Nov. 15, 2019, 5:06 a.m. UTC
From: P J P <pjp@fedoraproject.org>

openpic_src_write sets interrupt level 'src->output' masked with
ILR_INTTGT_MASK(=0xFF). It's then used to index 'dst->outputs_active'
array. With NUM_OUTPUTS=3, it may lead to OOB array access. Limit
active IRQ sources to < NUM_OUTPUTS.

Reported-by: Reno Robert <renorobert@gmail.com>
Signed-off-by: P J P <pjp@fedoraproject.org>
---
 arch/powerpc/kvm/mpic.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Update v2: limit IRQ sources to NUM_OUTPUTS
  -> https://www.spinics.net/lists/kvm-ppc/msg16554.html

--
2.21.0

Comments

Paul Mackerras Nov. 20, 2019, 2:33 a.m. UTC | #1
On Fri, Nov 15, 2019 at 10:36:20AM +0530, P J P wrote:
> From: P J P <pjp@fedoraproject.org>
> 
> openpic_src_write sets interrupt level 'src->output' masked with
> ILR_INTTGT_MASK(=0xFF). It's then used to index 'dst->outputs_active'
> array. With NUM_OUTPUTS=3, it may lead to OOB array access. Limit
> active IRQ sources to < NUM_OUTPUTS.
> 
> Reported-by: Reno Robert <renorobert@gmail.com>
> Signed-off-by: P J P <pjp@fedoraproject.org>
> ---
>  arch/powerpc/kvm/mpic.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> Update v2: limit IRQ sources to NUM_OUTPUTS
>   -> https://www.spinics.net/lists/kvm-ppc/msg16554.html
> 
> diff --git a/arch/powerpc/kvm/mpic.c b/arch/powerpc/kvm/mpic.c
> index fe312c160d97..fe4afd54c6e7 100644
> --- a/arch/powerpc/kvm/mpic.c
> +++ b/arch/powerpc/kvm/mpic.c
> @@ -628,7 +628,7 @@ static inline void write_IRQreg_ilr(struct openpic *opp, int n_IRQ,
>  	if (opp->flags & OPENPIC_FLAG_ILR) {
>  		struct irq_source *src = &opp->src[n_IRQ];
> 
> -		src->output = val & ILR_INTTGT_MASK;
> +		src->output = val % NUM_OUTPUTS;

Still not right, I'm afraid, since it could leave src->output set to
3, which would lead to an out-of-bounds array access.  I think it
needs to be

	if (val < NUM_OUTPUTS)
		src->output = val;
	else
		src->output = ILR_INTTGT_INT;

or something like that.

Paul.
P J P Nov. 20, 2019, 11 a.m. UTC | #2
+-- On Wed, 20 Nov 2019, Paul Mackerras wrote --+
| Still not right, I'm afraid, since it could leave src->output set to
| 3, which would lead to an out-of-bounds array access.  I think it
| needs to be

===
#include <stdio.h>

int
main (int argc, char *argv[])
{   
    int i = 0;

    for (i = 0; i < 1024; i++) {
        printf ("%d: %d\n", i, i % 0x3);
    }

    return 0;
}

-> https://paste.centos.org/view/fb14b3cf
===

It does not seem to set it to 0x3. When a no is divisible by 0x3, 
'src->output' will be set to zero(0).

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
Paul Mackerras Nov. 20, 2019, 9:41 p.m. UTC | #3
On Wed, Nov 20, 2019 at 04:30:32PM +0530, P J P wrote:
> +-- On Wed, 20 Nov 2019, Paul Mackerras wrote --+
> | Still not right, I'm afraid, since it could leave src->output set to
> | 3, which would lead to an out-of-bounds array access.  I think it
> | needs to be
> 
> ===
> #include <stdio.h>
> 
> int
> main (int argc, char *argv[])
> {   
>     int i = 0;
> 
>     for (i = 0; i < 1024; i++) {
>         printf ("%d: %d\n", i, i % 0x3);
>     }
> 
>     return 0;
> }
> 
> -> https://paste.centos.org/view/fb14b3cf
> ===
> 
> It does not seem to set it to 0x3. When a no is divisible by 0x3, 
> 'src->output' will be set to zero(0).

You're right, I misread the '%' as '&'.

Paul.
P J P Nov. 21, 2019, 4:57 a.m. UTC | #4
Hello Paul,

+-- On Thu, 21 Nov 2019, Paul Mackerras wrote --+
| > It does not seem to set it to 0x3. When a no is divisible by 0x3, 
| > 'src->output' will be set to zero(0).
| 
| You're right, I misread the '%' as '&'.

Considering E500 is mostly used for SoC/Embedded systems. It is not clear if 
this issue can be misused from a guest running on PPC E500 platform.

...wdyt?
--
Prasad J Pandit / Red Hat Product Security Team
8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
P J P Nov. 27, 2019, 8:24 a.m. UTC | #5
+-- On Thu, 21 Nov 2019, P J P wrote --+
| Considering E500 is mostly used for SoC/Embedded systems. It is not clear if 
| this issue can be misused from a guest running on PPC E500 platform.
| 
| ...wdyt?

Paul, any idea?
--
Prasad J Pandit / Red Hat Product Security Team
8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D

Patch
diff mbox series

diff --git a/arch/powerpc/kvm/mpic.c b/arch/powerpc/kvm/mpic.c
index fe312c160d97..fe4afd54c6e7 100644
--- a/arch/powerpc/kvm/mpic.c
+++ b/arch/powerpc/kvm/mpic.c
@@ -628,7 +628,7 @@  static inline void write_IRQreg_ilr(struct openpic *opp, int n_IRQ,
 	if (opp->flags & OPENPIC_FLAG_ILR) {
 		struct irq_source *src = &opp->src[n_IRQ];

-		src->output = val & ILR_INTTGT_MASK;
+		src->output = val % NUM_OUTPUTS;
 		pr_debug("Set ILR %d to 0x%08x, output %d\n", n_IRQ, src->idr,
 			src->output);