NetworkPkg/Ip6Dxe/Ip6Nd.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
This patch fixes reversed logic of recently added ASSERTs which should
ensure that Ip6IsNDOptionValid() implementation properly reacts to invalid
packets.
Cc: Jiaxin Wu <jiaxin.wu@intel.com>
Cc: Siyuan Fu <siyuan.fu@intel.com>
Signed-off-by: Maciej Rabeda <maciej.rabeda@linux.intel.com>
Tested-by: Laszlo Ersek <lersek@redhat.com>
---
NetworkPkg/Ip6Dxe/Ip6Nd.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/NetworkPkg/Ip6Dxe/Ip6Nd.c b/NetworkPkg/Ip6Dxe/Ip6Nd.c
index fd7f60b2f92c..0780a98cb325 100644
--- a/NetworkPkg/Ip6Dxe/Ip6Nd.c
+++ b/NetworkPkg/Ip6Dxe/Ip6Nd.c
@@ -2111,7 +2111,7 @@ Ip6ProcessRouterAdvertise (
// Option size validity ensured by Ip6IsNDOptionValid().
//
ASSERT (LinkLayerOption.Length != 0);
- ASSERT (Offset + (UINT32) LinkLayerOption.Length * 8 >= (UINT32) Head->PayloadLength);
+ ASSERT (Offset + (UINT32) LinkLayerOption.Length * 8 <= (UINT32) Head->PayloadLength);
ZeroMem (&LinkLayerAddress, sizeof (EFI_MAC_ADDRESS));
CopyMem (&LinkLayerAddress, LinkLayerOption.EtherAddr, 6);
@@ -2164,7 +2164,7 @@ Ip6ProcessRouterAdvertise (
// Option size validity ensured by Ip6IsNDOptionValid().
//
ASSERT (PrefixOption.Length == 4);
- ASSERT (Offset + (UINT32) PrefixOption.Length * 8 >= (UINT32) Head->PayloadLength);
+ ASSERT (Offset + (UINT32) PrefixOption.Length * 8 <= (UINT32) Head->PayloadLength);
PrefixOption.ValidLifetime = NTOHL (PrefixOption.ValidLifetime);
PrefixOption.PreferredLifetime = NTOHL (PrefixOption.PreferredLifetime);
@@ -2334,7 +2334,7 @@ Ip6ProcessRouterAdvertise (
// Option size validity ensured by Ip6IsNDOptionValid().
//
ASSERT (MTUOption.Length == 1);
- ASSERT (Offset + (UINT32) MTUOption.Length * 8 >= (UINT32) Head->PayloadLength);
+ ASSERT (Offset + (UINT32) MTUOption.Length * 8 <= (UINT32) Head->PayloadLength);
//
// Use IPv6 minimum link MTU 1280 bytes as the maximum packet size in order
--
2.24.0.windows.2
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#56829): https://edk2.groups.io/g/devel/message/56829
Mute This Topic: https://groups.io/mt/72696739/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Hi Maciej, On 04/01/20 11:53, Maciej Rabeda wrote: > This patch fixes reversed logic of recently added ASSERTs which should > ensure that Ip6IsNDOptionValid() implementation properly reacts to invalid > packets. > > Cc: Jiaxin Wu <jiaxin.wu@intel.com> > Cc: Siyuan Fu <siyuan.fu@intel.com> > Signed-off-by: Maciej Rabeda <maciej.rabeda@linux.intel.com> > Tested-by: Laszlo Ersek <lersek@redhat.com> > --- > NetworkPkg/Ip6Dxe/Ip6Nd.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) Thanks for the patch. Two meta-suggestions: (1) we should do one of the following: (1a) Create a new BZ for this issue, cross-link it -- via the See Also field -- with TianoCore#2174, and reference the new BZ in this commit message. If we file this new BZ, it should get the Regression keyword. (1b) Or else we should reopen TianoCore#2174, and reference *that* BZ in this commit message. (2) Independently of (1), the commit message should carry the following tag: Fixes: 9c20342eed70ec99ec50cd73cb81804299f05403 Regarding this patch, the above updates only affect the commit message, so I'm not asking for a v2 -- you can implement the commit message changes right before pushing. Thanks! Laszlo > diff --git a/NetworkPkg/Ip6Dxe/Ip6Nd.c b/NetworkPkg/Ip6Dxe/Ip6Nd.c > index fd7f60b2f92c..0780a98cb325 100644 > --- a/NetworkPkg/Ip6Dxe/Ip6Nd.c > +++ b/NetworkPkg/Ip6Dxe/Ip6Nd.c > @@ -2111,7 +2111,7 @@ Ip6ProcessRouterAdvertise ( > // Option size validity ensured by Ip6IsNDOptionValid(). > // > ASSERT (LinkLayerOption.Length != 0); > - ASSERT (Offset + (UINT32) LinkLayerOption.Length * 8 >= (UINT32) Head->PayloadLength); > + ASSERT (Offset + (UINT32) LinkLayerOption.Length * 8 <= (UINT32) Head->PayloadLength); > > ZeroMem (&LinkLayerAddress, sizeof (EFI_MAC_ADDRESS)); > CopyMem (&LinkLayerAddress, LinkLayerOption.EtherAddr, 6); > @@ -2164,7 +2164,7 @@ Ip6ProcessRouterAdvertise ( > // Option size validity ensured by Ip6IsNDOptionValid(). > // > ASSERT (PrefixOption.Length == 4); > - ASSERT (Offset + (UINT32) PrefixOption.Length * 8 >= (UINT32) Head->PayloadLength); > + ASSERT (Offset + (UINT32) PrefixOption.Length * 8 <= (UINT32) Head->PayloadLength); > > PrefixOption.ValidLifetime = NTOHL (PrefixOption.ValidLifetime); > PrefixOption.PreferredLifetime = NTOHL (PrefixOption.PreferredLifetime); > @@ -2334,7 +2334,7 @@ Ip6ProcessRouterAdvertise ( > // Option size validity ensured by Ip6IsNDOptionValid(). > // > ASSERT (MTUOption.Length == 1); > - ASSERT (Offset + (UINT32) MTUOption.Length * 8 >= (UINT32) Head->PayloadLength); > + ASSERT (Offset + (UINT32) MTUOption.Length * 8 <= (UINT32) Head->PayloadLength); > > // > // Use IPv6 minimum link MTU 1280 bytes as the maximum packet size in order > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#56843): https://edk2.groups.io/g/devel/message/56843 Mute This Topic: https://groups.io/mt/72696739/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
With Laszlo's comments. Reviewed-by: Siyuan Fu <siyuan.fu@intel.com> > -----Original Message----- > From: Laszlo Ersek <lersek@redhat.com> > Sent: 2020年4月1日 20:02 > To: Maciej Rabeda <maciej.rabeda@linux.intel.com>; devel@edk2.groups.io > Cc: Wu, Jiaxin <jiaxin.wu@intel.com>; Fu, Siyuan <siyuan.fu@intel.com> > Subject: Re: [PATCH] NetworkPkg/Ip6Dxe: Fix ASSERT logic in > Ip6ProcessRouterAdvertise() > > Hi Maciej, > > On 04/01/20 11:53, Maciej Rabeda wrote: > > This patch fixes reversed logic of recently added ASSERTs which should > > ensure that Ip6IsNDOptionValid() implementation properly reacts to > invalid > > packets. > > > > Cc: Jiaxin Wu <jiaxin.wu@intel.com> > > Cc: Siyuan Fu <siyuan.fu@intel.com> > > Signed-off-by: Maciej Rabeda <maciej.rabeda@linux.intel.com> > > Tested-by: Laszlo Ersek <lersek@redhat.com> > > --- > > NetworkPkg/Ip6Dxe/Ip6Nd.c | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > Thanks for the patch. Two meta-suggestions: > > (1) we should do one of the following: > > (1a) Create a new BZ for this issue, cross-link it -- via the See Also > field -- with TianoCore#2174, and reference the new BZ in this commit > message. If we file this new BZ, it should get the Regression keyword. > > (1b) Or else we should reopen TianoCore#2174, and reference *that* BZ in > this commit message. > > (2) Independently of (1), the commit message should carry the following tag: > > Fixes: 9c20342eed70ec99ec50cd73cb81804299f05403 > > Regarding this patch, the above updates only affect the commit message, > so I'm not asking for a v2 -- you can implement the commit message > changes right before pushing. > > Thanks! > Laszlo > > > > > diff --git a/NetworkPkg/Ip6Dxe/Ip6Nd.c b/NetworkPkg/Ip6Dxe/Ip6Nd.c > > index fd7f60b2f92c..0780a98cb325 100644 > > --- a/NetworkPkg/Ip6Dxe/Ip6Nd.c > > +++ b/NetworkPkg/Ip6Dxe/Ip6Nd.c > > @@ -2111,7 +2111,7 @@ Ip6ProcessRouterAdvertise ( > > // Option size validity ensured by Ip6IsNDOptionValid(). > > // > > ASSERT (LinkLayerOption.Length != 0); > > - ASSERT (Offset + (UINT32) LinkLayerOption.Length * 8 >= (UINT32) > Head->PayloadLength); > > + ASSERT (Offset + (UINT32) LinkLayerOption.Length * 8 <= (UINT32) > Head->PayloadLength); > > > > ZeroMem (&LinkLayerAddress, sizeof (EFI_MAC_ADDRESS)); > > CopyMem (&LinkLayerAddress, LinkLayerOption.EtherAddr, 6); > > @@ -2164,7 +2164,7 @@ Ip6ProcessRouterAdvertise ( > > // Option size validity ensured by Ip6IsNDOptionValid(). > > // > > ASSERT (PrefixOption.Length == 4); > > - ASSERT (Offset + (UINT32) PrefixOption.Length * 8 >= (UINT32) Head- > >PayloadLength); > > + ASSERT (Offset + (UINT32) PrefixOption.Length * 8 <= (UINT32) Head- > >PayloadLength); > > > > PrefixOption.ValidLifetime = NTOHL (PrefixOption.ValidLifetime); > > PrefixOption.PreferredLifetime = NTOHL > (PrefixOption.PreferredLifetime); > > @@ -2334,7 +2334,7 @@ Ip6ProcessRouterAdvertise ( > > // Option size validity ensured by Ip6IsNDOptionValid(). > > // > > ASSERT (MTUOption.Length == 1); > > - ASSERT (Offset + (UINT32) MTUOption.Length * 8 >= (UINT32) Head- > >PayloadLength); > > + ASSERT (Offset + (UINT32) MTUOption.Length * 8 <= (UINT32) Head- > >PayloadLength); > > > > // > > // Use IPv6 minimum link MTU 1280 bytes as the maximum packet size > in order > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#56881): https://edk2.groups.io/g/devel/message/56881 Mute This Topic: https://groups.io/mt/72696739/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
Hey Laszlo, Thanks for your _meta-suggestions_ :) (1a): BZ created, referenced to BZ 2174, marked with 'regression': https://bugzilla.tianocore.org/show_bug.cgi?id=2655 (2): This will occur upon passed review and pull request to edk2/master Siyuan/Jiaxin, Can we get this through? Thanks, Maciej On 01-Apr-20 14:02, Laszlo Ersek wrote: > Hi Maciej, > > On 04/01/20 11:53, Maciej Rabeda wrote: >> This patch fixes reversed logic of recently added ASSERTs which should >> ensure that Ip6IsNDOptionValid() implementation properly reacts to invalid >> packets. >> >> Cc: Jiaxin Wu <jiaxin.wu@intel.com> >> Cc: Siyuan Fu <siyuan.fu@intel.com> >> Signed-off-by: Maciej Rabeda <maciej.rabeda@linux.intel.com> >> Tested-by: Laszlo Ersek <lersek@redhat.com> >> --- >> NetworkPkg/Ip6Dxe/Ip6Nd.c | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) > Thanks for the patch. Two meta-suggestions: > > (1) we should do one of the following: > > (1a) Create a new BZ for this issue, cross-link it -- via the See Also > field -- with TianoCore#2174, and reference the new BZ in this commit > message. If we file this new BZ, it should get the Regression keyword. > > (1b) Or else we should reopen TianoCore#2174, and reference *that* BZ in > this commit message. > > (2) Independently of (1), the commit message should carry the following tag: > > Fixes: 9c20342eed70ec99ec50cd73cb81804299f05403 > > Regarding this patch, the above updates only affect the commit message, > so I'm not asking for a v2 -- you can implement the commit message > changes right before pushing. > > Thanks! > Laszlo > > > >> diff --git a/NetworkPkg/Ip6Dxe/Ip6Nd.c b/NetworkPkg/Ip6Dxe/Ip6Nd.c >> index fd7f60b2f92c..0780a98cb325 100644 >> --- a/NetworkPkg/Ip6Dxe/Ip6Nd.c >> +++ b/NetworkPkg/Ip6Dxe/Ip6Nd.c >> @@ -2111,7 +2111,7 @@ Ip6ProcessRouterAdvertise ( >> // Option size validity ensured by Ip6IsNDOptionValid(). >> // >> ASSERT (LinkLayerOption.Length != 0); >> - ASSERT (Offset + (UINT32) LinkLayerOption.Length * 8 >= (UINT32) Head->PayloadLength); >> + ASSERT (Offset + (UINT32) LinkLayerOption.Length * 8 <= (UINT32) Head->PayloadLength); >> >> ZeroMem (&LinkLayerAddress, sizeof (EFI_MAC_ADDRESS)); >> CopyMem (&LinkLayerAddress, LinkLayerOption.EtherAddr, 6); >> @@ -2164,7 +2164,7 @@ Ip6ProcessRouterAdvertise ( >> // Option size validity ensured by Ip6IsNDOptionValid(). >> // >> ASSERT (PrefixOption.Length == 4); >> - ASSERT (Offset + (UINT32) PrefixOption.Length * 8 >= (UINT32) Head->PayloadLength); >> + ASSERT (Offset + (UINT32) PrefixOption.Length * 8 <= (UINT32) Head->PayloadLength); >> >> PrefixOption.ValidLifetime = NTOHL (PrefixOption.ValidLifetime); >> PrefixOption.PreferredLifetime = NTOHL (PrefixOption.PreferredLifetime); >> @@ -2334,7 +2334,7 @@ Ip6ProcessRouterAdvertise ( >> // Option size validity ensured by Ip6IsNDOptionValid(). >> // >> ASSERT (MTUOption.Length == 1); >> - ASSERT (Offset + (UINT32) MTUOption.Length * 8 >= (UINT32) Head->PayloadLength); >> + ASSERT (Offset + (UINT32) MTUOption.Length * 8 <= (UINT32) Head->PayloadLength); >> >> // >> // Use IPv6 minimum link MTU 1280 bytes as the maximum packet size in order >> > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#56848): https://edk2.groups.io/g/devel/message/56848 Mute This Topic: https://groups.io/mt/72696739/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2024 Red Hat, Inc.