* [PATCH] NetworkPkg/Ip6Dxe: Fix ASSERT logic in Ip6ProcessRouterAdvertise()
@ 2020-04-01 9:53 Maciej Rabeda
2020-04-01 12:02 ` Laszlo Ersek
0 siblings, 1 reply; 4+ messages in thread
From: Maciej Rabeda @ 2020-04-01 9:53 UTC (permalink / raw)
To: devel; +Cc: Jiaxin Wu, Siyuan Fu, Laszlo Ersek
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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] NetworkPkg/Ip6Dxe: Fix ASSERT logic in Ip6ProcessRouterAdvertise()
2020-04-01 9:53 [PATCH] NetworkPkg/Ip6Dxe: Fix ASSERT logic in Ip6ProcessRouterAdvertise() Maciej Rabeda
@ 2020-04-01 12:02 ` Laszlo Ersek
2020-04-01 13:30 ` [edk2-devel] " Maciej Rabeda
2020-04-02 1:15 ` Siyuan, Fu
0 siblings, 2 replies; 4+ messages in thread
From: Laszlo Ersek @ 2020-04-01 12:02 UTC (permalink / raw)
To: Maciej Rabeda, devel; +Cc: Jiaxin Wu, Siyuan Fu
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
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [edk2-devel] [PATCH] NetworkPkg/Ip6Dxe: Fix ASSERT logic in Ip6ProcessRouterAdvertise()
2020-04-01 12:02 ` Laszlo Ersek
@ 2020-04-01 13:30 ` Maciej Rabeda
2020-04-02 1:15 ` Siyuan, Fu
1 sibling, 0 replies; 4+ messages in thread
From: Maciej Rabeda @ 2020-04-01 13:30 UTC (permalink / raw)
To: devel, lersek; +Cc: Jiaxin Wu, Siyuan Fu
[-- Attachment #1: Type: text/plain, Size: 3324 bytes --]
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
>>
>
>
>
[-- Attachment #2: Type: text/html, Size: 4336 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] NetworkPkg/Ip6Dxe: Fix ASSERT logic in Ip6ProcessRouterAdvertise()
2020-04-01 12:02 ` Laszlo Ersek
2020-04-01 13:30 ` [edk2-devel] " Maciej Rabeda
@ 2020-04-02 1:15 ` Siyuan, Fu
1 sibling, 0 replies; 4+ messages in thread
From: Siyuan, Fu @ 2020-04-02 1:15 UTC (permalink / raw)
To: Laszlo Ersek, Maciej Rabeda, devel@edk2.groups.io; +Cc: Wu, Jiaxin
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
> >
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-04-02 1:16 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-04-01 9:53 [PATCH] NetworkPkg/Ip6Dxe: Fix ASSERT logic in Ip6ProcessRouterAdvertise() Maciej Rabeda
2020-04-01 12:02 ` Laszlo Ersek
2020-04-01 13:30 ` [edk2-devel] " Maciej Rabeda
2020-04-02 1:15 ` Siyuan, Fu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox