| Message ID | 20191115050620.21360-1-ppandit@redhat.com |
|---|---|
| State | Rejected |
| Headers | show |
| Series | [v2] kvm: mpic: limit active IRQ sources to NUM_OUTPUTS | expand |
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.
+-- 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
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.
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
+-- 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
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);