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
> -----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>
在 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> > > . >
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>
> -----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.
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
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>
在 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. > > > > .
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.
> > 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