* [Qemu-devel] [PATCH 0/2] ARM SMMUv3: Fix a couple of Coverity issues @ 2018-05-16 18:03 Eric Auger 2018-05-16 18:03 ` [Qemu-devel] [PATCH 1/2] hw/arm/smmuv3: Fix Coverity issue in smmuv3_record_event Eric Auger ` (2 more replies) 0 siblings, 3 replies; 9+ messages in thread From: Eric Auger @ 2018-05-16 18:03 UTC (permalink / raw) To: eric.auger.pro, eric.auger, qemu-devel, qemu-arm, peter.maydell This series includes 2 patches that fix Coverity issues respectively in smmuv3 and smmu-common code. Eric Auger (2): hw/arm/smmuv3: Fix Coverity issue in smmuv3_record_event hw/arm/smmu-common: Fix coverity issue in get_block_pte_address hw/arm/smmu-common.c | 4 ++-- hw/arm/smmuv3.c | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) -- 1.8.3.1 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [Qemu-devel] [PATCH 1/2] hw/arm/smmuv3: Fix Coverity issue in smmuv3_record_event 2018-05-16 18:03 [Qemu-devel] [PATCH 0/2] ARM SMMUv3: Fix a couple of Coverity issues Eric Auger @ 2018-05-16 18:03 ` Eric Auger 2018-05-16 16:02 ` [Qemu-devel] [Qemu-arm] " Philippe Mathieu-Daudé 2018-05-16 18:03 ` [Qemu-devel] [PATCH 2/2] hw/arm/smmu-common: Fix coverity issue in get_block_pte_address Eric Auger 2018-05-17 15:37 ` [Qemu-devel] [PATCH 0/2] ARM SMMUv3: Fix a couple of Coverity issues Peter Maydell 2 siblings, 1 reply; 9+ messages in thread From: Eric Auger @ 2018-05-16 18:03 UTC (permalink / raw) To: eric.auger.pro, eric.auger, qemu-devel, qemu-arm, peter.maydell Coverity complains about use of uninitialized Evt struct. The EVT_SET_TYPE and similar setters use deposit32() on fields in the struct, so they read the uninitialized existing values. In cases where we don't set all the fields in the event struct we'll end up leaking random uninitialized data from QEMU's stack into the guest. Initializing the struct with "Evt evt = {};" ought to satisfy Coverity and fix the data leak. Signed-off-by: Eric Auger <eric.auger@redhat.com> Reported-by: Peter Maydell <peter.maydell@linaro.org> --- hw/arm/smmuv3.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c index b3026de..42dc521 100644 --- a/hw/arm/smmuv3.c +++ b/hw/arm/smmuv3.c @@ -143,7 +143,7 @@ static MemTxResult smmuv3_write_eventq(SMMUv3State *s, Evt *evt) void smmuv3_record_event(SMMUv3State *s, SMMUEventInfo *info) { - Evt evt; + Evt evt = {}; MemTxResult r; if (!smmuv3_eventq_enabled(s)) { -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [Qemu-arm] [PATCH 1/2] hw/arm/smmuv3: Fix Coverity issue in smmuv3_record_event 2018-05-16 18:03 ` [Qemu-devel] [PATCH 1/2] hw/arm/smmuv3: Fix Coverity issue in smmuv3_record_event Eric Auger @ 2018-05-16 16:02 ` Philippe Mathieu-Daudé 0 siblings, 0 replies; 9+ messages in thread From: Philippe Mathieu-Daudé @ 2018-05-16 16:02 UTC (permalink / raw) To: Eric Auger, eric.auger.pro, qemu-devel, qemu-arm, peter.maydell On 05/16/2018 03:03 PM, Eric Auger wrote: > Coverity complains about use of uninitialized Evt struct. > The EVT_SET_TYPE and similar setters use deposit32() on fields > in the struct, so they read the uninitialized existing values. > In cases where we don't set all the fields in the event struct > we'll end up leaking random uninitialized data from QEMU's > stack into the guest. > > Initializing the struct with "Evt evt = {};" ought to satisfy > Coverity and fix the data leak. > > Signed-off-by: Eric Auger <eric.auger@redhat.com> > Reported-by: Peter Maydell <peter.maydell@linaro.org> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- > hw/arm/smmuv3.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c > index b3026de..42dc521 100644 > --- a/hw/arm/smmuv3.c > +++ b/hw/arm/smmuv3.c > @@ -143,7 +143,7 @@ static MemTxResult smmuv3_write_eventq(SMMUv3State *s, Evt *evt) > > void smmuv3_record_event(SMMUv3State *s, SMMUEventInfo *info) > { > - Evt evt; > + Evt evt = {}; > MemTxResult r; > > if (!smmuv3_eventq_enabled(s)) { > ^ permalink raw reply [flat|nested] 9+ messages in thread
* [Qemu-devel] [PATCH 2/2] hw/arm/smmu-common: Fix coverity issue in get_block_pte_address 2018-05-16 18:03 [Qemu-devel] [PATCH 0/2] ARM SMMUv3: Fix a couple of Coverity issues Eric Auger 2018-05-16 18:03 ` [Qemu-devel] [PATCH 1/2] hw/arm/smmuv3: Fix Coverity issue in smmuv3_record_event Eric Auger @ 2018-05-16 18:03 ` Eric Auger 2018-05-16 16:16 ` [Qemu-devel] [Qemu-arm] " Philippe Mathieu-Daudé 2018-05-17 15:37 ` [Qemu-devel] [PATCH 0/2] ARM SMMUv3: Fix a couple of Coverity issues Peter Maydell 2 siblings, 1 reply; 9+ messages in thread From: Eric Auger @ 2018-05-16 18:03 UTC (permalink / raw) To: eric.auger.pro, eric.auger, qemu-devel, qemu-arm, peter.maydell Coverity points out that this can overflow if n > 31, because it's only doing 32-bit arithmetic. Let's use 1ULL instead of 1. Also the formulae used to compute n can be replaced by the level_shift() macro. Reported-by: Peter Maydell <peter.maydell@linaro.org> Signed-off-by: Eric Auger <eric.auger@redhat.com> --- hw/arm/smmu-common.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c index 01c7be8..3c5f724 100644 --- a/hw/arm/smmu-common.c +++ b/hw/arm/smmu-common.c @@ -83,9 +83,9 @@ static inline hwaddr get_table_pte_address(uint64_t pte, int granule_sz) static inline hwaddr get_block_pte_address(uint64_t pte, int level, int granule_sz, uint64_t *bsz) { - int n = (granule_sz - 3) * (4 - level) + 3; + int n = level_shift(level, granule_sz); - *bsz = 1 << n; + *bsz = 1ULL << n; return PTE_ADDRESS(pte, n); } -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [Qemu-arm] [PATCH 2/2] hw/arm/smmu-common: Fix coverity issue in get_block_pte_address 2018-05-16 18:03 ` [Qemu-devel] [PATCH 2/2] hw/arm/smmu-common: Fix coverity issue in get_block_pte_address Eric Auger @ 2018-05-16 16:16 ` Philippe Mathieu-Daudé 2018-05-16 16:23 ` Peter Maydell 0 siblings, 1 reply; 9+ messages in thread From: Philippe Mathieu-Daudé @ 2018-05-16 16:16 UTC (permalink / raw) To: Eric Auger, eric.auger.pro, qemu-devel, qemu-arm, peter.maydell Hi Eric, On 05/16/2018 03:03 PM, Eric Auger wrote: > Coverity points out that this can overflow if n > 31, > because it's only doing 32-bit arithmetic. Let's use 1ULL instead > of 1. Also the formulae used to compute n can be replaced by > the level_shift() macro. This level_shift() replacement doesn't seems that obvious to me, can you split it in another patch? > > Reported-by: Peter Maydell <peter.maydell@linaro.org> > Signed-off-by: Eric Auger <eric.auger@redhat.com> > --- > hw/arm/smmu-common.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c > index 01c7be8..3c5f724 100644 > --- a/hw/arm/smmu-common.c > +++ b/hw/arm/smmu-common.c > @@ -83,9 +83,9 @@ static inline hwaddr get_table_pte_address(uint64_t pte, int granule_sz) > static inline hwaddr get_block_pte_address(uint64_t pte, int level, > int granule_sz, uint64_t *bsz) > { > - int n = (granule_sz - 3) * (4 - level) + 3; > + int n = level_shift(level, granule_sz); Shouldn't this be level_shift(level + 1, granule_sz)? Using level_shift() you replaced the trailing 3 by granule_sz. This means the previous code was only correct for the granule_sz==3 case. level_shift(level + 1, granule_sz) == (granule_sz - 3) * (3 - (level + 1)) + granule_sz; == (granule_sz - 3) * (4 - level) + granule_sz; != (granule_sz - 3) * (4 - level) + 3; > > - *bsz = 1 << n; > + *bsz = 1ULL << n; For the coverity fix (patch splitted): Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > return PTE_ADDRESS(pte, n); > } > Regards, Phil. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [Qemu-arm] [PATCH 2/2] hw/arm/smmu-common: Fix coverity issue in get_block_pte_address 2018-05-16 16:16 ` [Qemu-devel] [Qemu-arm] " Philippe Mathieu-Daudé @ 2018-05-16 16:23 ` Peter Maydell 2018-05-16 20:01 ` Philippe Mathieu-Daudé 0 siblings, 1 reply; 9+ messages in thread From: Peter Maydell @ 2018-05-16 16:23 UTC (permalink / raw) To: Philippe Mathieu-Daudé Cc: Eric Auger, Eric Auger, QEMU Developers, qemu-arm On 16 May 2018 at 16:16, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > Hi Eric, > > On 05/16/2018 03:03 PM, Eric Auger wrote: >> Coverity points out that this can overflow if n > 31, >> because it's only doing 32-bit arithmetic. Let's use 1ULL instead >> of 1. Also the formulae used to compute n can be replaced by >> the level_shift() macro. > > This level_shift() replacement doesn't seems that obvious to me, can you > split it in another patch? > >> >> Reported-by: Peter Maydell <peter.maydell@linaro.org> >> Signed-off-by: Eric Auger <eric.auger@redhat.com> >> --- >> hw/arm/smmu-common.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c >> index 01c7be8..3c5f724 100644 >> --- a/hw/arm/smmu-common.c >> +++ b/hw/arm/smmu-common.c >> @@ -83,9 +83,9 @@ static inline hwaddr get_table_pte_address(uint64_t pte, int granule_sz) >> static inline hwaddr get_block_pte_address(uint64_t pte, int level, >> int granule_sz, uint64_t *bsz) >> { >> - int n = (granule_sz - 3) * (4 - level) + 3; >> + int n = level_shift(level, granule_sz); > > Shouldn't this be level_shift(level + 1, granule_sz)? No. The two expressions are equivalent, they're just arranged differently: level_shift(lvl, gsz) == gsz + (3 - lvl) * (gsz - 3) == gsz + (4 - lvl) * (gsz - 3) - (gsz - 3) == gsz - gsz + (4 - lvl) * (gsz - 3) + 3 == (gsz - 3) * (4 - lvl) + 3 thanks -- PMM ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [Qemu-arm] [PATCH 2/2] hw/arm/smmu-common: Fix coverity issue in get_block_pte_address 2018-05-16 16:23 ` Peter Maydell @ 2018-05-16 20:01 ` Philippe Mathieu-Daudé 2018-05-17 7:07 ` Auger Eric 0 siblings, 1 reply; 9+ messages in thread From: Philippe Mathieu-Daudé @ 2018-05-16 20:01 UTC (permalink / raw) To: Peter Maydell; +Cc: Eric Auger, Eric Auger, QEMU Developers, qemu-arm On 05/16/2018 01:23 PM, Peter Maydell wrote: > On 16 May 2018 at 16:16, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: >> Hi Eric, >> >> On 05/16/2018 03:03 PM, Eric Auger wrote: >>> Coverity points out that this can overflow if n > 31, >>> because it's only doing 32-bit arithmetic. Let's use 1ULL instead >>> of 1. Also the formulae used to compute n can be replaced by >>> the level_shift() macro. >> >> This level_shift() replacement doesn't seems that obvious to me, can you >> split it in another patch? >> >>> >>> Reported-by: Peter Maydell <peter.maydell@linaro.org> >>> Signed-off-by: Eric Auger <eric.auger@redhat.com> >>> --- >>> hw/arm/smmu-common.c | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c >>> index 01c7be8..3c5f724 100644 >>> --- a/hw/arm/smmu-common.c >>> +++ b/hw/arm/smmu-common.c >>> @@ -83,9 +83,9 @@ static inline hwaddr get_table_pte_address(uint64_t pte, int granule_sz) >>> static inline hwaddr get_block_pte_address(uint64_t pte, int level, >>> int granule_sz, uint64_t *bsz) >>> { >>> - int n = (granule_sz - 3) * (4 - level) + 3; >>> + int n = level_shift(level, granule_sz); >> >> Shouldn't this be level_shift(level + 1, granule_sz)? > > No. The two expressions are equivalent, they're > just arranged differently: > > level_shift(lvl, gsz) > == gsz + (3 - lvl) * (gsz - 3) > == gsz + (4 - lvl) * (gsz - 3) - (gsz - 3) > == gsz - gsz + (4 - lvl) * (gsz - 3) + 3 > == (gsz - 3) * (4 - lvl) + 3 Argh I failed this middle school demonstrations... Thanks Peter :) So for the much cleaner level_shift() use: Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [Qemu-arm] [PATCH 2/2] hw/arm/smmu-common: Fix coverity issue in get_block_pte_address 2018-05-16 20:01 ` Philippe Mathieu-Daudé @ 2018-05-17 7:07 ` Auger Eric 0 siblings, 0 replies; 9+ messages in thread From: Auger Eric @ 2018-05-17 7:07 UTC (permalink / raw) To: Philippe Mathieu-Daudé, Peter Maydell Cc: qemu-arm, QEMU Developers, Eric Auger Hi Philippe, On 05/16/2018 10:01 PM, Philippe Mathieu-Daudé wrote: > On 05/16/2018 01:23 PM, Peter Maydell wrote: >> On 16 May 2018 at 16:16, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: >>> Hi Eric, >>> >>> On 05/16/2018 03:03 PM, Eric Auger wrote: >>>> Coverity points out that this can overflow if n > 31, >>>> because it's only doing 32-bit arithmetic. Let's use 1ULL instead >>>> of 1. Also the formulae used to compute n can be replaced by >>>> the level_shift() macro. >>> >>> This level_shift() replacement doesn't seems that obvious to me, can you >>> split it in another patch? >>> >>>> >>>> Reported-by: Peter Maydell <peter.maydell@linaro.org> >>>> Signed-off-by: Eric Auger <eric.auger@redhat.com> >>>> --- >>>> hw/arm/smmu-common.c | 4 ++-- >>>> 1 file changed, 2 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c >>>> index 01c7be8..3c5f724 100644 >>>> --- a/hw/arm/smmu-common.c >>>> +++ b/hw/arm/smmu-common.c >>>> @@ -83,9 +83,9 @@ static inline hwaddr get_table_pte_address(uint64_t pte, int granule_sz) >>>> static inline hwaddr get_block_pte_address(uint64_t pte, int level, >>>> int granule_sz, uint64_t *bsz) >>>> { >>>> - int n = (granule_sz - 3) * (4 - level) + 3; >>>> + int n = level_shift(level, granule_sz); >>> >>> Shouldn't this be level_shift(level + 1, granule_sz)? >> >> No. The two expressions are equivalent, they're >> just arranged differently: >> >> level_shift(lvl, gsz) >> == gsz + (3 - lvl) * (gsz - 3) >> == gsz + (4 - lvl) * (gsz - 3) - (gsz - 3) >> == gsz - gsz + (4 - lvl) * (gsz - 3) + 3 >> == (gsz - 3) * (4 - lvl) + 3 > > Argh I failed this middle school demonstrations... > > Thanks Peter :) > > So for the much cleaner level_shift() use: > Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> Thank you for the review! Eric > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH 0/2] ARM SMMUv3: Fix a couple of Coverity issues 2018-05-16 18:03 [Qemu-devel] [PATCH 0/2] ARM SMMUv3: Fix a couple of Coverity issues Eric Auger 2018-05-16 18:03 ` [Qemu-devel] [PATCH 1/2] hw/arm/smmuv3: Fix Coverity issue in smmuv3_record_event Eric Auger 2018-05-16 18:03 ` [Qemu-devel] [PATCH 2/2] hw/arm/smmu-common: Fix coverity issue in get_block_pte_address Eric Auger @ 2018-05-17 15:37 ` Peter Maydell 2 siblings, 0 replies; 9+ messages in thread From: Peter Maydell @ 2018-05-17 15:37 UTC (permalink / raw) To: Eric Auger; +Cc: Eric Auger, QEMU Developers, qemu-arm On 16 May 2018 at 19:03, Eric Auger <eric.auger@redhat.com> wrote: > This series includes 2 patches that fix Coverity issues respectively > in smmuv3 and smmu-common code. > > Eric Auger (2): > hw/arm/smmuv3: Fix Coverity issue in smmuv3_record_event > hw/arm/smmu-common: Fix coverity issue in get_block_pte_address > > hw/arm/smmu-common.c | 4 ++-- > hw/arm/smmuv3.c | 2 +- > 2 files changed, 3 insertions(+), 3 deletions(-) > > -- Applied to target-arm.next, thanks. -- PMM ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2018-05-17 15:38 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-05-16 18:03 [Qemu-devel] [PATCH 0/2] ARM SMMUv3: Fix a couple of Coverity issues Eric Auger 2018-05-16 18:03 ` [Qemu-devel] [PATCH 1/2] hw/arm/smmuv3: Fix Coverity issue in smmuv3_record_event Eric Auger 2018-05-16 16:02 ` [Qemu-devel] [Qemu-arm] " Philippe Mathieu-Daudé 2018-05-16 18:03 ` [Qemu-devel] [PATCH 2/2] hw/arm/smmu-common: Fix coverity issue in get_block_pte_address Eric Auger 2018-05-16 16:16 ` [Qemu-devel] [Qemu-arm] " Philippe Mathieu-Daudé 2018-05-16 16:23 ` Peter Maydell 2018-05-16 20:01 ` Philippe Mathieu-Daudé 2018-05-17 7:07 ` Auger Eric 2018-05-17 15:37 ` [Qemu-devel] [PATCH 0/2] ARM SMMUv3: Fix a couple of Coverity issues Peter Maydell
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.