[dpdk-dev] [PATCH] lib/librte_power: fix using variables before validity check

[dpdk-dev] [PATCH] lib/librte_power: fix using variables before validity check

[Min Hu (Connor)]

From: HongBo Zheng <zhenghongbo3@huawei.com>

In function power_guest_channel_read_msg, 'lcore_id' is used before
validity check, which may cause buffer 'global_fds' accessed by index
'lcore_id' overflow.

This patch moves the validity check of 'lcore_id' before the 'lcore_id'
being used for the first time.

Fixes: 9dc843eb273b ("power: extend guest channel API for reading")
Cc: stable@dpdk.org

Signed-off-by: HongBo Zheng <zhenghongbo3@huawei.com>
Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
---
 lib/power/guest_channel.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/lib/power/guest_channel.c b/lib/power/guest_channel.c
index 2f7507a..d3de3f1 100644
--- a/lib/power/guest_channel.c
+++ b/lib/power/guest_channel.c
@@ -166,6 +166,12 @@ int power_guest_channel_read_msg(void *pkt,
 	if (pkt_len == 0 || pkt == NULL)
 		return -1;
 
+	if (lcore_id >= RTE_MAX_LCORE) {
+		RTE_LOG(ERR, GUEST_CHANNEL, "Channel(%u) is out of range 0...%d\n",
+				lcore_id, RTE_MAX_LCORE-1);
+		return -1;
+	}
+
 	fds.fd = global_fds[lcore_id];
 	fds.events = POLLIN;
 
@@ -179,12 +185,6 @@ int power_guest_channel_read_msg(void *pkt,
 		return -1;
 	}
 
-	if (lcore_id >= RTE_MAX_LCORE) {
-		RTE_LOG(ERR, GUEST_CHANNEL, "Channel(%u) is out of range 0...%d\n",
-				lcore_id, RTE_MAX_LCORE-1);
-		return -1;
-	}
-
 	if (global_fds[lcore_id] < 0) {
 		RTE_LOG(ERR, GUEST_CHANNEL, "Channel is not connected\n");
 		return -1;
-- 
2.7.4

Re: [dpdk-dev] [PATCH] lib/librte_power: fix using variables before validity check

[Pattan, Reshma]

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Min Hu (Connor)
> 
> +	if (lcore_id >= RTE_MAX_LCORE) {
> +		RTE_LOG(ERR, GUEST_CHANNEL, "Channel(%u) is out of range
> 0...%d\n",
> +				lcore_id, RTE_MAX_LCORE-1);
> +		return -1;
> +	}
> +

This looks good.

>  	fds.fd = global_fds[lcore_id];
>  	fds.events = POLLIN;
> 
<snip>

>  	if (global_fds[lcore_id] < 0) {
>  		RTE_LOG(ERR, GUEST_CHANNEL, "Channel is not
> connected\n");


Another suggestion if you would like to improve the code, is below,
You can move "global_fds[lcore_id]"  check to immediate after  the line  "fds.fd = global_fds[lcore_id]; "

Reviewed-by: Reshma Pattan <reshma.pattan@intel.com>

Re: [dpdk-dev] [PATCH] lib/librte_power: fix using variables before validity check

[Min Hu (Connor)]

在 2021/4/28 23:13, Pattan, Reshma 写道:
> 
> 
>> -----Original Message-----
>> From: dev <dev-bounces@dpdk.org> On Behalf Of Min Hu (Connor)
>>
>> +	if (lcore_id >= RTE_MAX_LCORE) {
>> +		RTE_LOG(ERR, GUEST_CHANNEL, "Channel(%u) is out of range
>> 0...%d\n",
>> +				lcore_id, RTE_MAX_LCORE-1);
>> +		return -1;
>> +	}
>> +
> 
> This looks good.
> 
>>   	fds.fd = global_fds[lcore_id];
>>   	fds.events = POLLIN;
>>
> <snip>
> 
>>   	if (global_fds[lcore_id] < 0) {
>>   		RTE_LOG(ERR, GUEST_CHANNEL, "Channel is not
>> connected\n");
> 
> 
> Another suggestion if you would like to improve the code, is below,
> You can move "global_fds[lcore_id]"  check to immediate after  the line  "fds.fd = global_fds[lcore_id]; "
Hi, thanks Pattan.
But I think "global_fds[lcore_id]"  check may move before the line 
"fds.fd = global_fds[lcore_id];
If it failed, "fds.fd = global_fds[lcore_id];" should not be performed.
What's your opinion?

> 
> Reviewed-by: Reshma Pattan <reshma.pattan@intel.com>
> 
> .
> 

Re: [dpdk-dev] [PATCH] lib/librte_power: fix using variables before validity check

[Thomas Monjalon]

Any comment please?

29/04/2021 02:43, Min Hu (Connor):
> 
> 在 2021/4/28 23:13, Pattan, Reshma 写道:
> > 
> > 
> >> -----Original Message-----
> >> From: dev <dev-bounces@dpdk.org> On Behalf Of Min Hu (Connor)
> >>
> >> +	if (lcore_id >= RTE_MAX_LCORE) {
> >> +		RTE_LOG(ERR, GUEST_CHANNEL, "Channel(%u) is out of range
> >> 0...%d\n",
> >> +				lcore_id, RTE_MAX_LCORE-1);
> >> +		return -1;
> >> +	}
> >> +
> > 
> > This looks good.
> > 
> >>   	fds.fd = global_fds[lcore_id];
> >>   	fds.events = POLLIN;
> >>
> > <snip>
> > 
> >>   	if (global_fds[lcore_id] < 0) {
> >>   		RTE_LOG(ERR, GUEST_CHANNEL, "Channel is not
> >> connected\n");
> > 
> > 
> > Another suggestion if you would like to improve the code, is below,
> > You can move "global_fds[lcore_id]"  check to immediate after  the line  "fds.fd = global_fds[lcore_id]; "
> Hi, thanks Pattan.
> But I think "global_fds[lcore_id]"  check may move before the line 
> "fds.fd = global_fds[lcore_id];
> If it failed, "fds.fd = global_fds[lcore_id];" should not be performed.
> What's your opinion?
> 
> > 
> > Reviewed-by: Reshma Pattan <reshma.pattan@intel.com>

Re: [dpdk-dev] [PATCH] lib/librte_power: fix using variables before validity check

[Pattan, Reshma]

> -----Original Message-----
> From: Min Hu (Connor) <humin29@huawei.com>
> > Another suggestion if you would like to improve the code, is below,
> > You can move "global_fds[lcore_id]"  check to immediate after  the line
> "fds.fd = global_fds[lcore_id]; "
> Hi, thanks Pattan.
> But I think "global_fds[lcore_id]"  check may move before the line
> "fds.fd = global_fds[lcore_id];
> If it failed, "fds.fd = global_fds[lcore_id];" should not be performed.
> What's your opinion?
> 

That is correct.  You can do that change.

[dpdk-dev] [PATCH v2] lib/librte_power: fix using variables before validity check

[Min Hu (Connor)]

From: HongBo Zheng <zhenghongbo3@huawei.com>

In function power_guest_channel_read_msg, 'lcore_id' is used before
validity check, which may cause buffer 'global_fds' accessed by index
'lcore_id' overflow.

This patch moves the validity check of 'lcore_id' before the 'lcore_id'
being used for the first time.

Fixes: 9dc843eb273b ("power: extend guest channel API for reading")
Cc: stable@dpdk.org

Signed-off-by: HongBo Zheng <zhenghongbo3@huawei.com>
Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
---
v2:
* "global_fds[lcore_id]"  check may move before the line
"fds.fd = global_fds[lcore_id].
---
 lib/power/guest_channel.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/lib/power/guest_channel.c b/lib/power/guest_channel.c
index 2f7507a..474dd92 100644
--- a/lib/power/guest_channel.c
+++ b/lib/power/guest_channel.c
@@ -166,6 +166,17 @@ int power_guest_channel_read_msg(void *pkt,
 	if (pkt_len == 0 || pkt == NULL)
 		return -1;
 
+	if (lcore_id >= RTE_MAX_LCORE) {
+		RTE_LOG(ERR, GUEST_CHANNEL, "Channel(%u) is out of range 0...%d\n",
+				lcore_id, RTE_MAX_LCORE-1);
+		return -1;
+	}
+
+	if (global_fds[lcore_id] < 0) {
+		RTE_LOG(ERR, GUEST_CHANNEL, "Channel is not connected\n");
+		return -1;
+	}
+
 	fds.fd = global_fds[lcore_id];
 	fds.events = POLLIN;
 
@@ -179,17 +190,6 @@ int power_guest_channel_read_msg(void *pkt,
 		return -1;
 	}
 
-	if (lcore_id >= RTE_MAX_LCORE) {
-		RTE_LOG(ERR, GUEST_CHANNEL, "Channel(%u) is out of range 0...%d\n",
-				lcore_id, RTE_MAX_LCORE-1);
-		return -1;
-	}
-
-	if (global_fds[lcore_id] < 0) {
-		RTE_LOG(ERR, GUEST_CHANNEL, "Channel is not connected\n");
-		return -1;
-	}
-
 	while (pkt_len > 0) {
 		ret = read(global_fds[lcore_id],
 				pkt, pkt_len);
-- 
2.7.4

Re: [dpdk-dev] [PATCH v2] lib/librte_power: fix using variables before validity check

[David Hunt]

On 12/5/2021 3:19 AM, Min Hu (Connor) wrote:
> From: HongBo Zheng <zhenghongbo3@huawei.com>
>
> In function power_guest_channel_read_msg, 'lcore_id' is used before
> validity check, which may cause buffer 'global_fds' accessed by index
> 'lcore_id' overflow.
>
> This patch moves the validity check of 'lcore_id' before the 'lcore_id'
> being used for the first time.
>
> Fixes: 9dc843eb273b ("power: extend guest channel API for reading")
> Cc: stable@dpdk.org
>
> Signed-off-by: HongBo Zheng <zhenghongbo3@huawei.com>
> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
> ---
> v2:
> * "global_fds[lcore_id]"  check may move before the line
> "fds.fd = global_fds[lcore_id].


Hi Connor,

Just for future reference, it is common to include tags from previous 
version of a patch set unless there's major changes. So it would have 
been good to include Reshma's "Reviewed-by" tag in v2.

Acked-by: David Hunt <david.hunt@intel.com>

Re: [dpdk-dev] [PATCH v2] lib/librte_power: fix using variables before validity check

[Min Hu (Connor)]

在 2021/5/12 15:03, David Hunt 写道:
> 
> On 12/5/2021 3:19 AM, Min Hu (Connor) wrote:
>> From: HongBo Zheng <zhenghongbo3@huawei.com>
>>
>> In function power_guest_channel_read_msg, 'lcore_id' is used before
>> validity check, which may cause buffer 'global_fds' accessed by index
>> 'lcore_id' overflow.
>>
>> This patch moves the validity check of 'lcore_id' before the 'lcore_id'
>> being used for the first time.
>>
>> Fixes: 9dc843eb273b ("power: extend guest channel API for reading")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: HongBo Zheng <zhenghongbo3@huawei.com>
>> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
>> ---
>> v2:
>> * "global_fds[lcore_id]"  check may move before the line
>> "fds.fd = global_fds[lcore_id].
> 
> 
> Hi Connor,
> 
> Just for future reference, it is common to include tags from previous 
> version of a patch set unless there's major changes. So it would have 
> been good to include Reshma's "Reviewed-by" tag in v2.
> 
> Acked-by: David Hunt <david.hunt@intel.com>
Thanks David, got it.
> 
> 
> 
> .

Re: [dpdk-dev] [PATCH v2] lib/librte_power: fix using variables before validity check

[David Hunt]

On 12/5/2021 8:14 AM, Min Hu (Connor) wrote:
>
>
> 在 2021/5/12 15:03, David Hunt 写道:
>>
>> On 12/5/2021 3:19 AM, Min Hu (Connor) wrote:
>>> From: HongBo Zheng <zhenghongbo3@huawei.com>
>>>
>>> In function power_guest_channel_read_msg, 'lcore_id' is used before
>>> validity check, which may cause buffer 'global_fds' accessed by index
>>> 'lcore_id' overflow.
>>>
>>> This patch moves the validity check of 'lcore_id' before the 'lcore_id'
>>> being used for the first time.
>>>
>>> Fixes: 9dc843eb273b ("power: extend guest channel API for reading")
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: HongBo Zheng <zhenghongbo3@huawei.com>
>>> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
>>> ---
>>> v2:
>>> * "global_fds[lcore_id]"  check may move before the line
>>> "fds.fd = global_fds[lcore_id].
>>
>>
>> Hi Connor,
>>
>> Just for future reference, it is common to include tags from previous 
>> version of a patch set unless there's major changes. So it would have 
>> been good to include Reshma's "Reviewed-by" tag in v2.
>>
>> Acked-by: David Hunt <david.hunt@intel.com>
> Thanks David, got it.
>>
>>
>>
>> .


Hi Connor,

One more thing, when you put up a new version of a patch or patch-set, 
it's good to mark the previous as "Superseded" in patchwork.

http://patchwork.dpdk.org/project/dpdk/list/?series=16657

Rgds,

Dave.

Re: [dpdk-dev] [PATCH v2] lib/librte_power: fix using variables before validity check

[Thomas Monjalon]

> > In function power_guest_channel_read_msg, 'lcore_id' is used before
> > validity check, which may cause buffer 'global_fds' accessed by index
> > 'lcore_id' overflow.
> >
> > This patch moves the validity check of 'lcore_id' before the 'lcore_id'
> > being used for the first time.
> >
> > Fixes: 9dc843eb273b ("power: extend guest channel API for reading")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: HongBo Zheng <zhenghongbo3@huawei.com>
> > Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
> > ---
> > v2:
> > * "global_fds[lcore_id]"  check may move before the line
> > "fds.fd = global_fds[lcore_id].
> 
> 
> Hi Connor,
> 
> Just for future reference, it is common to include tags from previous 
> version of a patch set unless there's major changes. So it would have 
> been good to include Reshma's "Reviewed-by" tag in v2.
> 
> Acked-by: David Hunt <david.hunt@intel.com>

Applied with Reshma's tag, thanks.
title: power: fix sanity checks for guest channel read