Re: [Patch] MdeModulePkg: Add wrong/invalid subnet check

[Laszlo Ersek <lersek@redhat.com>]

I have a few comments:

On 11/10/16 09:45, Jiaxin Wu wrote:
> This patch is used to add the wrong/invalid subnet check.
> Meanwhile, correct the the return status.

(1) I propose to split this patch into three patches, with the following
subjects:

MdeModulePkg/Ip4Dxe: Catch invalid subnet early in manual setting
MdeModulePkg/Ip4Dxe: Fix error path return status
MdeModulePkg/Ip4Dxe: Catch invalid subnet in Ip4SetAddress() helper

In this structuring, patch #1 would be actually redundant; patch #3
would handle that case automatically. But, we can keep all three if you
wish.

> 
> Cc: Santhapur Naveen <naveens@amiindia.co.in>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ye Ting <ting.ye@intel.com>
> Cc: Fu Siyuan <siyuan.fu@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Jiaxin Wu <jiaxin.wu@intel.com>
> ---
>  MdeModulePkg/Universal/Network/Ip4Dxe/Ip4Config2Impl.c | 18 +++++++++++-------
>  MdeModulePkg/Universal/Network/Ip4Dxe/Ip4If.c          |  8 +++++---
>  2 files changed, 16 insertions(+), 10 deletions(-)
> 
> diff --git a/MdeModulePkg/Universal/Network/Ip4Dxe/Ip4Config2Impl.c b/MdeModulePkg/Universal/Network/Ip4Dxe/Ip4Config2Impl.c
> index a931bb3..672a092 100644
> --- a/MdeModulePkg/Universal/Network/Ip4Dxe/Ip4Config2Impl.c
> +++ b/MdeModulePkg/Universal/Network/Ip4Dxe/Ip4Config2Impl.c
> @@ -1253,10 +1253,17 @@ Ip4Config2SetMaunualAddress (
>      return EFI_WRITE_PROTECTED;
>    }
>  
>    NewAddress = *((EFI_IP4_CONFIG2_MANUAL_ADDRESS *) Data);
>  
> +  StationAddress = EFI_NTOHL (NewAddress.Address);
> +  SubnetMask = EFI_NTOHL (NewAddress.SubnetMask);
> +
> +  if (NetGetMaskLength (SubnetMask) > IP4_MASK_MAX) {
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
>    //
>    // Store the new data, and init the DataItem status to EFI_NOT_READY because
>    // we may have an asynchronous configuration process.
>    //
>    Ptr = AllocateCopyPool (DataSize, Data);
> @@ -1271,30 +1278,27 @@ Ip4Config2SetMaunualAddress (
>    
>    DataItem->Data.Ptr = Ptr;
>    DataItem->DataSize = DataSize;
>    DataItem->Status   = EFI_NOT_READY;
>  
> -  StationAddress = EFI_NTOHL (NewAddress.Address);
> -  SubnetMask = EFI_NTOHL (NewAddress.SubnetMask);
> -

(2) This part looks good to me, but for stylistic reasons, I recommend
replacing

  > IP4_MASK_MAX

with

  == IP4_MASK_NUM

The reason is that the leading comment on NetGetMaskLength() documents
IP4_MASK_NUM as the error value:

  @return The length of the netmask, IP4_MASK_NUM if the mask is invalid.

Okay, so this is where patch #2 should start:

>    IpSb->Reconfig = TRUE;
>    Status = Ip4Config2SetDefaultAddr (IpSb, StationAddress, SubnetMask);
>    if (EFI_ERROR (Status)) {
>      goto ON_EXIT;
>    }  
>  
> -  DataItem->Status = EFI_SUCCESS;   
> -
>  ON_EXIT:
> -  if (EFI_ERROR (DataItem->Status)) {
> +  DataItem->Status = Status; 
> +
> +  if (EFI_ERROR (DataItem->Status) && DataItem->Status != EFI_NOT_READY) {
>      if (Ptr != NULL) {
>        FreePool (Ptr);
>      }
>      DataItem->Data.Ptr = NULL; 
>    }
>  
> -  return EFI_SUCCESS;
> +  return Status;
>  }
>  
>  /**
>    The work function is to set the gateway addresses manually for the EFI IPv4 
>    network stack that is running on the communication device that this EFI IPv4 

(3) This looks good (with your explanation in the bugzilla), except you
could remove the ON_EXIT label too, with the referring goto statement as
well. There is nothing left between the (sole) jump to ON_EXIT, and
ON_EXIT itself, so the goto is useless.

This is where the third patch should start:

> diff --git a/MdeModulePkg/Universal/Network/Ip4Dxe/Ip4If.c b/MdeModulePkg/Universal/Network/Ip4Dxe/Ip4If.c
> index 9cd5dd5..7550a13 100644
> --- a/MdeModulePkg/Universal/Network/Ip4Dxe/Ip4If.c
> +++ b/MdeModulePkg/Universal/Network/Ip4Dxe/Ip4If.c
> @@ -562,10 +562,15 @@ Ip4SetAddress (
>    EFI_STATUS                Status;
>    INTN                      Len;
>  
>    NET_CHECK_SIGNATURE (Interface, IP4_INTERFACE_SIGNATURE);
>  
> +  Len = NetGetMaskLength (SubnetMask);
> +  if (Len > IP4_MASK_MAX) {
> +    return EFI_INVALID_PARAMETER;
> +  }
> +

(4) Same comment as (2), about IP4_MASK_NUM.

>    //
>    // Set the ip/netmask, then compute the subnet broadcast
>    // and network broadcast for easy access. When computing
>    // nework broadcast, the subnet mask is most like longer
>    // than the default netmask (not subneted) as defined in
> @@ -573,13 +578,10 @@ Ip4SetAddress (
>    // networks, use the subnet's mask instead.
>    //
>    Interface->Ip             = IpAddr;
>    Interface->SubnetMask     = SubnetMask;
>    Interface->SubnetBrdcast  = (IpAddr | ~SubnetMask);
> -
> -  Len                       = NetGetMaskLength (SubnetMask);
> -  ASSERT (Len <= IP4_MASK_MAX);
>    Interface->NetBrdcast     = (IpAddr | ~SubnetMask);
>  
>    //
>    // Do clean up for Arp child
>    //
> 

Thanks!
Laszlo