[Intel-wired-lan] [PATCH v2 1/1] e1000e: Fix possible overflow in LTR decoding

[Intel-wired-lan] [PATCH v2 1/1] e1000e: Fix possible overflow in LTR decoding

[Sasha Neftin]

When we decode the latency and the max_latency u16 value does not fill
the required size and could lead to the wrong LTR representation.
Replace the u16 type with the u32 type and allow corrected LTR
representation.

Fixes: 44a13a5d99c7 ("e1000e: Fix the max snoop/no-snoop latency for 10M")
Reported-by: James Hutchinson <jahutchinson99@googlemail.com>
Link: https://bugzilla.kernel.org/show_bug.cgi?id=215689
Suggested-by: Dima Ruinskiy <dima.ruinskiy@intel.com>
Signed-off-by: Sasha Neftin <sasha.neftin@intel.com>
---
v2: added link tag

 drivers/net/ethernet/intel/e1000e/ich8lan.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/e1000e/ich8lan.c b/drivers/net/ethernet/intel/e1000e/ich8lan.c
index d60e2016d03c..e6c8e6d5234f 100644
--- a/drivers/net/ethernet/intel/e1000e/ich8lan.c
+++ b/drivers/net/ethernet/intel/e1000e/ich8lan.c
@@ -1009,8 +1009,8 @@ static s32 e1000_platform_pm_pch_lpt(struct e1000_hw *hw, bool link)
 {
 	u32 reg = link << (E1000_LTRV_REQ_SHIFT + E1000_LTRV_NOSNOOP_SHIFT) |
 	    link << E1000_LTRV_REQ_SHIFT | E1000_LTRV_SEND;
-	u16 max_ltr_enc_d = 0;	/* maximum LTR decoded by platform */
-	u16 lat_enc_d = 0;	/* latency decoded */
+	u32 max_ltr_enc_d = 0;	/* maximum LTR decoded by platform */
+	u32 lat_enc_d = 0;	/* latency decoded */
 	u16 lat_enc = 0;	/* latency encoded */
 
 	if (link) {
-- 
2.30.2

[Intel-wired-lan] [PATCH v2 1/1] e1000e: Fix possible overflow in LTR decoding

[naamax.meir]

On 4/5/2022 18:56, Sasha Neftin wrote:
> When we decode the latency and the max_latency u16 value does not fill
> the required size and could lead to the wrong LTR representation.
> Replace the u16 type with the u32 type and allow corrected LTR
> representation.
> 
> Fixes: 44a13a5d99c7 ("e1000e: Fix the max snoop/no-snoop latency for 10M")
> Reported-by: James Hutchinson <jahutchinson99@googlemail.com>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=215689
> Suggested-by: Dima Ruinskiy <dima.ruinskiy@intel.com>
> Signed-off-by: Sasha Neftin <sasha.neftin@intel.com>
> ---
> v2: added link tag
> 
>   drivers/net/ethernet/intel/e1000e/ich8lan.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
Tested-by: Naama Meir <naamax.meir@linux.intel.com>

[Intel-wired-lan] [PATCH v2 1/1] e1000e: Fix possible overflow in LTR decoding

[Paul Menzel]

Dear Sasha,


Thank you for your patch.

Am 05.04.22 um 17:56 schrieb Sasha Neftin:
> When we decode the latency and the max_latency u16 value does not fill
> the required size

Do you mean ?fit into? or ?is too small for the required size??

> and could lead to the wrong LTR representation.

Maybe give an example of values leading to incorrect behavior?

> Replace the u16 type with the u32 type and allow corrected LTR
> representation.

Maybe: Increase the variable size from u16 to u32, so the decoded 
latency can be represented. Why are 32 bit enough? Why not 64 bit?

Please use 75 characters per line.

> Fixes: 44a13a5d99c7 ("e1000e: Fix the max snoop/no-snoop latency for 10M")
> Reported-by: James Hutchinson <jahutchinson99@googlemail.com>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=215689
> Suggested-by: Dima Ruinskiy <dima.ruinskiy@intel.com>
> Signed-off-by: Sasha Neftin <sasha.neftin@intel.com>

Add

Tested-by: James Hutchinson <jahutchinson99@googlemail.com> (I219-V (rev 
30))

> ---
> v2: added link tag
> 
>   drivers/net/ethernet/intel/e1000e/ich8lan.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/e1000e/ich8lan.c b/drivers/net/ethernet/intel/e1000e/ich8lan.c
> index d60e2016d03c..e6c8e6d5234f 100644
> --- a/drivers/net/ethernet/intel/e1000e/ich8lan.c
> +++ b/drivers/net/ethernet/intel/e1000e/ich8lan.c
> @@ -1009,8 +1009,8 @@ static s32 e1000_platform_pm_pch_lpt(struct e1000_hw *hw, bool link)
>   {
>   	u32 reg = link << (E1000_LTRV_REQ_SHIFT + E1000_LTRV_NOSNOOP_SHIFT) |
>   	    link << E1000_LTRV_REQ_SHIFT | E1000_LTRV_SEND;
> -	u16 max_ltr_enc_d = 0;	/* maximum LTR decoded by platform */
> -	u16 lat_enc_d = 0;	/* latency decoded */
> +	u32 max_ltr_enc_d = 0;	/* maximum LTR decoded by platform */
> +	u32 lat_enc_d = 0;	/* latency decoded */
>   	u16 lat_enc = 0;	/* latency encoded */
>   
>   	if (link) {

The diff looks good.


Kind regards,

Paul

[Intel-wired-lan] [PATCH v2 1/1] e1000e: Fix possible overflow in LTR decoding

[Neftin, Sasha]

On 4/6/2022 08:34, Paul Menzel wrote:
> Dear Sasha,
> 
> 
> Thank you for your patch.
> 
> Am 05.04.22 um 17:56 schrieb Sasha Neftin:
>> When we decode the latency and the max_latency u16 value does not fill
>> the required size
> 
> Do you mean ?fit into? or ?is too small for the required size??
> 
>> and could lead to the wrong LTR representation.
> 
> Maybe give an example of values leading to incorrect behavior?
> 
>> Replace the u16 type with the u32 type and allow corrected LTR
>> representation.
> 
> Maybe: Increase the variable size from u16 to u32, so the decoded 
> latency can be represented. Why are 32 bit enough? Why not 64 bit?
Hello Paul,
Scaling represented as:
scale 0 - 1         (2^(5*0)) = 2^0
scale 1 - 32        (2^(5 *1))= 2^5
scale 2 - 1024      (2^(5 *2)) =2^10
scale 3 - 32768     (2^(5 *3)) =2^15
scale 4 ? 1048576   (2^(5 *4)) = 2^20
scale 5 ? 33554432  (2^(5 *4)) = 2^25
scale 4 and scale 5 required 20 and 25 bits respectively.
scale 6 reserved.
> 
> Please use 75 characters per line.
> 
>> Fixes: 44a13a5d99c7 ("e1000e: Fix the max snoop/no-snoop latency for 
>> 10M")
>> Reported-by: James Hutchinson <jahutchinson99@googlemail.com>
>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=215689
>> Suggested-by: Dima Ruinskiy <dima.ruinskiy@intel.com>
>> Signed-off-by: Sasha Neftin <sasha.neftin@intel.com>
> 
> Add
> 
> Tested-by: James Hutchinson <jahutchinson99@googlemail.com> (I219-V (rev 
> 30))
I will let James add this tag.
> 
>> ---
>> v2: added link tag
>>
>> ? drivers/net/ethernet/intel/e1000e/ich8lan.c | 4 ++--
>> ? 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/e1000e/ich8lan.c 
>> b/drivers/net/ethernet/intel/e1000e/ich8lan.c
>> index d60e2016d03c..e6c8e6d5234f 100644
>> --- a/drivers/net/ethernet/intel/e1000e/ich8lan.c
>> +++ b/drivers/net/ethernet/intel/e1000e/ich8lan.c
>> @@ -1009,8 +1009,8 @@ static s32 e1000_platform_pm_pch_lpt(struct 
>> e1000_hw *hw, bool link)
>> ? {
>> ????? u32 reg = link << (E1000_LTRV_REQ_SHIFT + 
>> E1000_LTRV_NOSNOOP_SHIFT) |
>> ????????? link << E1000_LTRV_REQ_SHIFT | E1000_LTRV_SEND;
>> -??? u16 max_ltr_enc_d = 0;??? /* maximum LTR decoded by platform */
>> -??? u16 lat_enc_d = 0;??? /* latency decoded */
>> +??? u32 max_ltr_enc_d = 0;??? /* maximum LTR decoded by platform */
>> +??? u32 lat_enc_d = 0;??? /* latency decoded */
>> ????? u16 lat_enc = 0;??? /* latency encoded */
>> ????? if (link) {
> 
> The diff looks good.
Thanks
> 
> 
> Kind regards,
> 
> Paul
Sasha

[Intel-wired-lan] [PATCH v2 1/1] e1000e: Fix possible overflow in LTR decoding

[James Hutchinson]

On Wed, 6 Apr 2022, 15:37 Neftin, Sasha, <sasha.neftin@intel.com> wrote:

> On 4/6/2022 08:34, Paul Menzel wrote:
> > Dear Sasha,
> >
> >
> > Thank you for your patch.
> >
> > Am 05.04.22 um 17:56 schrieb Sasha Neftin:
> >> When we decode the latency and the max_latency u16 value does not fill
> >> the required size
> >
> > Do you mean ?fit into? or ?is too small for the required size??
> >
> >> and could lead to the wrong LTR representation.
> >
> > Maybe give an example of values leading to incorrect behavior?
> >
> >> Replace the u16 type with the u32 type and allow corrected LTR
> >> representation.
> >
> > Maybe: Increase the variable size from u16 to u32, so the decoded
> > latency can be represented. Why are 32 bit enough? Why not 64 bit?
> Hello Paul,
> Scaling represented as:
> scale 0 - 1         (2^(5*0)) = 2^0
> scale 1 - 32        (2^(5 *1))= 2^5
> scale 2 - 1024      (2^(5 *2)) =2^10
> scale 3 - 32768     (2^(5 *3)) =2^15
> scale 4 ? 1048576   (2^(5 *4)) = 2^20
> scale 5 ? 33554432  (2^(5 *4)) = 2^25
> scale 4 and scale 5 required 20 and 25 bits respectively.
> scale 6 reserved.
> >
> > Please use 75 characters per line.
> >
> >> Fixes: 44a13a5d99c7 ("e1000e: Fix the max snoop/no-snoop latency for
> >> 10M")
> >> Reported-by: James Hutchinson <jahutchinson99@googlemail.com>
> >> Link: https://bugzilla.kernel.org/show_bug.cgi?id=215689
> >> Suggested-by: Dima Ruinskiy <dima.ruinskiy@intel.com>
> >> Signed-off-by: Sasha Neftin <sasha.neftin@intel.com>
> >
> > Add
> >
> > Tested-by: James Hutchinson <jahutchinson99@googlemail.com> (I219-V
> (rev
> > 30))
> I will let James add this tag.
> >
> >> ---
> >> v2: added link tag
> >>
> >>   drivers/net/ethernet/intel/e1000e/ich8lan.c | 4 ++--
> >>   1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/net/ethernet/intel/e1000e/ich8lan.c
> >> b/drivers/net/ethernet/intel/e1000e/ich8lan.c
> >> index d60e2016d03c..e6c8e6d5234f 100644
> >> --- a/drivers/net/ethernet/intel/e1000e/ich8lan.c
> >> +++ b/drivers/net/ethernet/intel/e1000e/ich8lan.c
> >> @@ -1009,8 +1009,8 @@ static s32 e1000_platform_pm_pch_lpt(struct
> >> e1000_hw *hw, bool link)
> >>   {
> >>       u32 reg = link << (E1000_LTRV_REQ_SHIFT +
> >> E1000_LTRV_NOSNOOP_SHIFT) |
> >>           link << E1000_LTRV_REQ_SHIFT | E1000_LTRV_SEND;
> >> -    u16 max_ltr_enc_d = 0;    /* maximum LTR decoded by platform */
> >> -    u16 lat_enc_d = 0;    /* latency decoded */
> >> +    u32 max_ltr_enc_d = 0;    /* maximum LTR decoded by platform */
> >> +    u32 lat_enc_d = 0;    /* latency decoded */
> >>       u16 lat_enc = 0;    /* latency encoded */
> >>       if (link) {
> >
> > The diff looks good.
> Thanks
> >
> >
> > Kind regards,
> >
> > Paul
> Sasha
>
Tested-by: James Hutchinson <jahutchinson99@googlemail.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osuosl.org/pipermail/intel-wired-lan/attachments/20220406/0bda17e6/attachment-0001.html>

[Intel-wired-lan] [PATCH v2 1/1] e1000e: Fix possible overflow in LTR decoding

[Paul Menzel]

Dear Sasha, dear Tony,


Am 06.04.22 um 16:33 schrieb Neftin, Sasha:
> On 4/6/2022 08:34, Paul Menzel wrote:

>> Thank you for your patch.
>>
>> Am 05.04.22 um 17:56 schrieb Sasha Neftin:
>>> When we decode the latency and the max_latency u16 value does not fill
>>> the required size
>>
>> Do you mean ?fit into? or ?is too small for the required size??

Tony, I saw you committed this patch [1]. Is it still possible to fix 
the wording?

>>> and could lead to the wrong LTR representation.
>>
>> Maybe give an example of values leading to incorrect behavior?
>>
>>> Replace the u16 type with the u32 type and allow corrected LTR
>>> representation.
>>
>> Maybe: Increase the variable size from u16 to u32, so the decoded 
>> latency can be represented. Why are 32 bit enough? Why not 64 bit?
> Hello Paul,
> Scaling represented as:
> scale 0 - 1???????? (2^(5*0)) = 2^0
> scale 1 - 32??????? (2^(5 *1))= 2^5
> scale 2 - 1024????? (2^(5 *2)) =2^10
> scale 3 - 32768???? (2^(5 *3)) =2^15
> scale 4 ? 1048576?? (2^(5 *4)) = 2^20
> scale 5 ? 33554432? (2^(5 *4)) = 2^25
> scale 4 and scale 5 required 20 and 25 bits respectively.
> scale 6 reserved.

This would have been nice in the commit message.

>> Please use 75 characters per line.
>>
>>> Fixes: 44a13a5d99c7 ("e1000e: Fix the max snoop/no-snoop latency for 
>>> 10M")
>>> Reported-by: James Hutchinson <jahutchinson99@googlemail.com>
>>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=215689
>>> Suggested-by: Dima Ruinskiy <dima.ruinskiy@intel.com>
>>> Signed-off-by: Sasha Neftin <sasha.neftin@intel.com>
>>
>> Add
>>
>> Tested-by: James Hutchinson <jahutchinson99@googlemail.com> (I219-V (rev 30))
> I will let James add this tag.
>>
>>> ---
>>> v2: added link tag
>>>
>>> ? drivers/net/ethernet/intel/e1000e/ich8lan.c | 4 ++--
>>> ? 1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/intel/e1000e/ich8lan.c 
>>> b/drivers/net/ethernet/intel/e1000e/ich8lan.c
>>> index d60e2016d03c..e6c8e6d5234f 100644
>>> --- a/drivers/net/ethernet/intel/e1000e/ich8lan.c
>>> +++ b/drivers/net/ethernet/intel/e1000e/ich8lan.c
>>> @@ -1009,8 +1009,8 @@ static s32 e1000_platform_pm_pch_lpt(struct 
>>> e1000_hw *hw, bool link)
>>> ? {
>>> ????? u32 reg = link << (E1000_LTRV_REQ_SHIFT + E1000_LTRV_NOSNOOP_SHIFT) |
>>> ????????? link << E1000_LTRV_REQ_SHIFT | E1000_LTRV_SEND;
>>> -??? u16 max_ltr_enc_d = 0;??? /* maximum LTR decoded by platform */
>>> -??? u16 lat_enc_d = 0;??? /* latency decoded */
>>> +??? u32 max_ltr_enc_d = 0;??? /* maximum LTR decoded by platform */
>>> +??? u32 lat_enc_d = 0;??? /* latency decoded */
>>> ????? u16 lat_enc = 0;??? /* latency encoded */
>>> ????? if (link) {
>>
>> The diff looks good.
> Thanks


Kind regards,

Paul


[1]: 
https://git.kernel.org/pub/scm/linux/kernel/git/tnguy/next-queue.git/commit/?h=dev-queue&id=7dd121b8d5735780b6a70db735d44b3e5b856130

[Intel-wired-lan] [PATCH v2 1/1] e1000e: Fix possible overflow in LTR decoding

[Tony Nguyen]

On 4/10/2022 3:10 AM, Paul Menzel wrote:
> Dear Sasha, dear Tony,
>
>
> Am 06.04.22 um 16:33 schrieb Neftin, Sasha:
>> On 4/6/2022 08:34, Paul Menzel wrote:
>
>>> Thank you for your patch.
>>>
>>> Am 05.04.22 um 17:56 schrieb Sasha Neftin:
>>>> When we decode the latency and the max_latency u16 value does not fill
>>>> the required size
>>>
>>> Do you mean ?fit into? or ?is too small for the required size??
>
> Tony, I saw you committed this patch [1]. Is it still possible to fix 
> the wording?

Yes, it can still be changed. I'll check with Sasha on any edits he 
wants to make before sending on to netdev.

Thanks,

Tony

>
>>>> and could lead to the wrong LTR representation.
>>>
>>> Maybe give an example of values leading to incorrect behavior?
>>>
>>>> Replace the u16 type with the u32 type and allow corrected LTR
>>>> representation.
>>>
>>> Maybe: Increase the variable size from u16 to u32, so the decoded 
>>> latency can be represented. Why are 32 bit enough? Why not 64 bit?
>> Hello Paul,
>> Scaling represented as:
>> scale 0 - 1???????? (2^(5*0)) = 2^0
>> scale 1 - 32??????? (2^(5 *1))= 2^5
>> scale 2 - 1024????? (2^(5 *2)) =2^10
>> scale 3 - 32768???? (2^(5 *3)) =2^15
>> scale 4 ? 1048576?? (2^(5 *4)) = 2^20
>> scale 5 ? 33554432? (2^(5 *4)) = 2^25
>> scale 4 and scale 5 required 20 and 25 bits respectively.
>> scale 6 reserved.
>
> This would have been nice in the commit message.
>
>>> Please use 75 characters per line.
>>>
>>>> Fixes: 44a13a5d99c7 ("e1000e: Fix the max snoop/no-snoop latency 
>>>> for 10M")
>>>> Reported-by: James Hutchinson <jahutchinson99@googlemail.com>
>>>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=215689
>>>> Suggested-by: Dima Ruinskiy <dima.ruinskiy@intel.com>
>>>> Signed-off-by: Sasha Neftin <sasha.neftin@intel.com>
>>>
>>> Add
>>>
>>> Tested-by: James Hutchinson <jahutchinson99@googlemail.com> (I219-V 
>>> (rev 30))
>> I will let James add this tag.
>>>
>>>> ---
>>>> v2: added link tag
>>>>
>>>> ? drivers/net/ethernet/intel/e1000e/ich8lan.c | 4 ++--
>>>> ? 1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/net/ethernet/intel/e1000e/ich8lan.c 
>>>> b/drivers/net/ethernet/intel/e1000e/ich8lan.c
>>>> index d60e2016d03c..e6c8e6d5234f 100644
>>>> --- a/drivers/net/ethernet/intel/e1000e/ich8lan.c
>>>> +++ b/drivers/net/ethernet/intel/e1000e/ich8lan.c
>>>> @@ -1009,8 +1009,8 @@ static s32 e1000_platform_pm_pch_lpt(struct 
>>>> e1000_hw *hw, bool link)
>>>> ? {
>>>> ????? u32 reg = link << (E1000_LTRV_REQ_SHIFT + 
>>>> E1000_LTRV_NOSNOOP_SHIFT) |
>>>> ????????? link << E1000_LTRV_REQ_SHIFT | E1000_LTRV_SEND;
>>>> -??? u16 max_ltr_enc_d = 0;??? /* maximum LTR decoded by platform */
>>>> -??? u16 lat_enc_d = 0;??? /* latency decoded */
>>>> +??? u32 max_ltr_enc_d = 0;??? /* maximum LTR decoded by platform */
>>>> +??? u32 lat_enc_d = 0;??? /* latency decoded */
>>>> ????? u16 lat_enc = 0;??? /* latency encoded */
>>>> ????? if (link) {
>>>
>>> The diff looks good.
>> Thanks
>
>
> Kind regards,
>
> Paul
>
>
> [1]: 
> https://git.kernel.org/pub/scm/linux/kernel/git/tnguy/next-queue.git/commit/?h=dev-queue&id=7dd121b8d5735780b6a70db735d44b3e5b856130