[v1] examples/vm_power_manager: fix buffer overrun
Checks
Commit Message
The freqs array in freq_info struct has RTE_MAX_LCORE_FREQS elements,
yet the code can attemtp to look at the index at POWER_MANAGER_MAX_CPUS,
which may be greater than RTE_MAX_LCORE_FREQS. Fix to limit index to
RTE_MAX_LCORE_FREQS.
Fixes: d26c18c93260 ("examples/vm_power: cpu frequency in host")
Coverity issue: 337660
Signed-off-by: David Hunt <david.hunt@intel.com>
---
examples/vm_power_manager/power_manager.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Comments
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of David Hunt
> Sent: Wednesday, April 10, 2019 1:49 PM
> To: dev@dpdk.org
> Cc: Hunt, David <david.hunt@intel.com>; stable@dpdk.org
> Subject: [dpdk-dev] [PATCH v1] examples/vm_power_manager: fix buffer
> overrun
>
> The freqs array in freq_info struct has RTE_MAX_LCORE_FREQS elements, yet
> the code can attemtp to look at the index at POWER_MANAGER_MAX_CPUS,
Couple of nit-picks,
:%s/ attempt/ attempt
> which may be greater than RTE_MAX_LCORE_FREQS. Fix to limit index to
> RTE_MAX_LCORE_FREQS.
>
> Fixes: d26c18c93260 ("examples/vm_power: cpu frequency in host")
> Coverity issue: 337660
Candidate for stable@dpdk.org?
Acked-by: Reshma Pattan <reshma.pattan@intel.com>
Thanks,
Reshma
10/04/2019 14:49, David Hunt:
> The freqs array in freq_info struct has RTE_MAX_LCORE_FREQS elements,
> yet the code can attemtp to look at the index at POWER_MANAGER_MAX_CPUS,
> which may be greater than RTE_MAX_LCORE_FREQS. Fix to limit index to
> RTE_MAX_LCORE_FREQS.
>
> Fixes: d26c18c93260 ("examples/vm_power: cpu frequency in host")
> Coverity issue: 337660
>
> Signed-off-by: David Hunt <david.hunt@intel.com>
It seems to have been fixed in another patch, isn't it?
Hi Thomas,
On 22/4/2019 10:54 PM, Thomas Monjalon wrote:
> 10/04/2019 14:49, David Hunt:
>> The freqs array in freq_info struct has RTE_MAX_LCORE_FREQS elements,
>> yet the code can attemtp to look at the index at POWER_MANAGER_MAX_CPUS,
>> which may be greater than RTE_MAX_LCORE_FREQS. Fix to limit index to
>> RTE_MAX_LCORE_FREQS.
>>
>> Fixes: d26c18c93260 ("examples/vm_power: cpu frequency in host")
>> Coverity issue: 337660
>>
>> Signed-off-by: David Hunt <david.hunt@intel.com>
> It seems to have been fixed in another patch, isn't it?
>
It was not fixed in another patch, although I can see the confusion.
A previous patch made the #defines more consistent, and
POWER_MGR_MAX_CPUS was changed to RTE_MAX_LCORE on the affected line.
However, this was later revealed as a coverity issue, and was fixed in
this patch to be RTE_LCORE_MAX_FREQS, which is the size of the array
it's trying to index into.
So looking at RC2, this patch is still needed.
Rgds,
Dave.
23/04/2019 10:21, Hunt, David:
> Hi Thomas,
>
> On 22/4/2019 10:54 PM, Thomas Monjalon wrote:
> > 10/04/2019 14:49, David Hunt:
> >> The freqs array in freq_info struct has RTE_MAX_LCORE_FREQS elements,
> >> yet the code can attemtp to look at the index at POWER_MANAGER_MAX_CPUS,
> >> which may be greater than RTE_MAX_LCORE_FREQS. Fix to limit index to
> >> RTE_MAX_LCORE_FREQS.
> >>
> >> Fixes: d26c18c93260 ("examples/vm_power: cpu frequency in host")
> >> Coverity issue: 337660
> >>
> >> Signed-off-by: David Hunt <david.hunt@intel.com>
> > It seems to have been fixed in another patch, isn't it?
> >
>
> It was not fixed in another patch, although I can see the confusion.
>
> A previous patch made the #defines more consistent, and
> POWER_MGR_MAX_CPUS was changed to RTE_MAX_LCORE on the affected line.
> However, this was later revealed as a coverity issue, and was fixed in
> this patch to be RTE_LCORE_MAX_FREQS, which is the size of the array
> it's trying to index into.
>
> So looking at RC2, this patch is still needed.
I think it needs to be rebased in a v2 then.
On 23/4/2019 9:33 AM, Thomas Monjalon wrote:
> 23/04/2019 10:21, Hunt, David:
>> Hi Thomas,
>>
>> On 22/4/2019 10:54 PM, Thomas Monjalon wrote:
>>> 10/04/2019 14:49, David Hunt:
>>>> The freqs array in freq_info struct has RTE_MAX_LCORE_FREQS elements,
>>>> yet the code can attemtp to look at the index at POWER_MANAGER_MAX_CPUS,
>>>> which may be greater than RTE_MAX_LCORE_FREQS. Fix to limit index to
>>>> RTE_MAX_LCORE_FREQS.
>>>>
>>>> Fixes: d26c18c93260 ("examples/vm_power: cpu frequency in host")
>>>> Coverity issue: 337660
>>>>
>>>> Signed-off-by: David Hunt <david.hunt@intel.com>
>>> It seems to have been fixed in another patch, isn't it?
>>>
>> It was not fixed in another patch, although I can see the confusion.
>>
>> A previous patch made the #defines more consistent, and
>> POWER_MGR_MAX_CPUS was changed to RTE_MAX_LCORE on the affected line.
>> However, this was later revealed as a coverity issue, and was fixed in
>> this patch to be RTE_LCORE_MAX_FREQS, which is the size of the array
>> it's trying to index into.
>>
>> So looking at RC2, this patch is still needed.
> I think it needs to be rebased in a v2 then.
>
Sure, will do.
On 18/04/2019 16:14, Pattan, Reshma wrote:
>> Fixes: d26c18c93260 ("examples/vm_power: cpu frequency in host")
>> Coverity issue: 337660
> Candidate for stable@dpdk.org?
There is no reply to this comment - re-asking as I will probably have
the same question in a few weeks :-)
Hi Kevin,
On 23/4/2019 11:26 AM, Kevin Traynor wrote:
> On 18/04/2019 16:14, Pattan, Reshma wrote:
>>> Fixes: d26c18c93260 ("examples/vm_power: cpu frequency in host")
>>> Coverity issue: 337660
>> Candidate for stable@dpdk.org?
> There is no reply to this comment - re-asking as I will probably have
> the same question in a few weeks :-)
There was a v2 this morning and it had a --cc stable on it. :)
Rgds,
Dave.
On 23/04/2019 11:31, Hunt, David wrote:
> Hi Kevin,
>
> On 23/4/2019 11:26 AM, Kevin Traynor wrote:
>> On 18/04/2019 16:14, Pattan, Reshma wrote:
>>>> Fixes: d26c18c93260 ("examples/vm_power: cpu frequency in host")
>>>> Coverity issue: 337660
>>> Candidate for stable@dpdk.org?
>> There is no reply to this comment - re-asking as I will probably have
>> the same question in a few weeks :-)
>
>
> There was a v2 this morning and it had a --cc stable on it. :)
>
Yes, you're right, but that just puts it in the email and it gets lost
from the commit message. The 'Cc: stable@dpdk.org' tag is needed in the
commit message so it can be picked up by scripts later when finding
which patches should be backported.
https://doc.dpdk.org/guides/contributing/stable.html#what-changes-should-be-backported
As it's discussed, maybe Thomas can add it for this patch when applying.
thanks,
Kevin.
> Rgds,
> Dave.
>
@@ -143,7 +143,7 @@ power_manager_get_current_frequency(unsigned core_num)
rte_spinlock_lock(&global_core_freq_info[core_num].power_sl);
index = rte_power_get_freq(core_num);
rte_spinlock_unlock(&global_core_freq_info[core_num].power_sl);
- if (index >= POWER_MGR_MAX_CPUS)
+ if (index >= RTE_MAX_LCORE_FREQS)
freq = 0;
else
freq = global_core_freq_info[core_num].freqs[index];