| Message ID | 20220405155601.1443799-1-sasha.neftin@intel.com |
|---|---|
| State | Accepted |
| Delegated to: | Anthony Nguyen |
| Headers | show |
| Series | [v2,1/1] e1000e: Fix possible overflow in LTR decoding | expand |
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>
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
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
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>
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
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
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) {
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(-)