Re: [edk2-devel] [PATCH] NetworkPkg/Ip6Dxe: Fix ASSERT logic in Ip6ProcessRouterAdvertise()

["Maciej Rabeda" <maciej.rabeda@linux.intel.com>]

[-- 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 --]