| Message ID | 1526493784-25328-3-git-send-email-eric.auger@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | ARM SMMUv3: Fix a couple of Coverity issues | expand |
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.
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
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>
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 >
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); }
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(-)