Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1452765 > unrolled thread

[PATCH] net: thunderx: correct bound check in nic_config_loopback

Back to article view | Back to linux.kernel

Contents

  [PATCH] net: thunderx: correct bound check in nic_config_loopback "Levin, Alexander" <alexander.levin@verizon.com> - 2016-07-31 05:00 +0200
    Re: [PATCH] net: thunderx: correct bound check in nic_config_loopback Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> - 2016-07-31 12:00 +0200
    Re: [PATCH] net: thunderx: correct bound check in nic_config_loopback Sunil Kovvuri <sunil.kovvuri@gmail.com> - 2016-07-31 18:50 +0200
      Re: [PATCH] net: thunderx: correct bound check in  nic_config_loopback "Levin, Alexander" <alexander.levin@verizon.com> - 2016-08-01 18:10 +0200
        Re: [PATCH] net: thunderx: correct bound check in nic_config_loopback Sunil Kovvuri <sunil.kovvuri@gmail.com> - 2016-08-02 13:50 +0200

#1452765 — [PATCH] net: thunderx: correct bound check in nic_config_loopback

Off by one in nic_config_loopback would access an invalid arrat variable when
vf id == MAX_LMAC.

Signed-off-by: Sasha Levin <alexander.levin@verizon.com>
---
 drivers/net/ethernet/cavium/thunder/nic_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/cavium/thunder/nic_main.c b/drivers/net/ethernet/cavium/thunder/nic_main.c
index 16ed203..a70f50d 100644
--- a/drivers/net/ethernet/cavium/thunder/nic_main.c
+++ b/drivers/net/ethernet/cavium/thunder/nic_main.c
@@ -615,7 +615,7 @@ static int nic_config_loopback(struct nicpf *nic, struct set_loopback *lbk)
 {
 	int bgx_idx, lmac_idx;
 
-	if (lbk->vf_id > MAX_LMAC)
+	if (lbk->vf_id >= MAX_LMAC)
 		return -1;
 
 	bgx_idx = NIC_GET_BGX_FROM_VF_LMAC_MAP(nic->vf_lmac_map[lbk->vf_id]);
-- 
2.7.4

[toc] | [next] | [standalone]

#1452798

Hello.

On 7/31/2016 5:49 AM, Levin, Alexander wrote:

> Off by one in nic_config_loopback would access an invalid arrat variable when

    Array?

> vf id == MAX_LMAC.
>
> Signed-off-by: Sasha Levin <alexander.levin@verizon.com>
[...]

MBR, Sergei

[toc] | [prev] | [next] | [standalone]

#1452860

Thanks for finding.
A much better fix would be,

-       if (lbk->vf_id > MAX_LMAC)
+       if (lbk->vf_id >= nic->num_vf_en)
                return -1;

where 'num_vf_en' reflects the exact number of physical interfaces or
LMACs on the system.

Thanks,
Sunil.

[toc] | [prev] | [next] | [standalone]

#1453294 — Re: [PATCH] net: thunderx: correct bound check in nic_config_loopback

On 07/31/2016 12:41 PM, Sunil Kovvuri wrote:
> Thanks for finding.
> A much better fix would be,
> 
> -       if (lbk->vf_id > MAX_LMAC)
> +       if (lbk->vf_id >= nic->num_vf_en)
>                 return -1;
> 
> where 'num_vf_en' reflects the exact number of physical interfaces or
> LMACs on the system.

Right. I see quite a few more places that compare to MAX_LMAC vs
num_vf_en. What was the reasoning behind it then?


Thanks,
Sasha

[toc] | [prev] | [next] | [standalone]

#1453887

[Multipart message — attachments visible in raw view] — view raw

Yes, it's incorrect at other places as well.
That went in the very early stages of development and didn't change it because
that out of bounds issue will never happen as no of logical interfaces
will never be
morethan  MAX_LMAC i.e 8, so max vf_id will be 7.

But with addition of support for newer platforms with different set HW
capabilities we
are slowly getting rid of most of the macros i.e static info.
Attached is the patch which will get rid of MAX_LMAC and also allows
support for 16LMACs (supported on newer platforms) or more.

I hope currently you are not facing any issue with below check.
>>> if (lbk->vf_id > MAX_LMAC)

I will submit the attached patch along with other patches when net-next is open.
https://lkml.org/lkml/2016/7/15/362

Thanks,
Sunil.

On Mon, Aug 1, 2016 at 9:27 PM, Levin, Alexander
<alexander.levin@verizon.com> wrote:
> On 07/31/2016 12:41 PM, Sunil Kovvuri wrote:
>> Thanks for finding.
>> A much better fix would be,
>>
>> -       if (lbk->vf_id > MAX_LMAC)
>> +       if (lbk->vf_id >= nic->num_vf_en)
>>                 return -1;
>>
>> where 'num_vf_en' reflects the exact number of physical interfaces or
>> LMACs on the system.
>
> Right. I see quite a few more places that compare to MAX_LMAC vs
> num_vf_en. What was the reasoning behind it then?
>
>
> Thanks,
> Sasha

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web