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

["Wu, Jiaxin" <jiaxin.wu@intel.com>]

Thanks Laszlo, your suggestions make sense.

Best Regards!
Jiaxin

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Thursday, November 10, 2016 5:19 PM
> To: Wu, Jiaxin <jiaxin.wu@intel.com>; edk2-devel@ml01.01.org
> Cc: Santhapur Naveen <naveens@amiindia.co.in>; Ye, Ting
> <ting.ye@intel.com>; Fu, Siyuan <siyuan.fu@intel.com>
> Subject: Re: [Patch] MdeModulePkg: Add wrong/invalid subnet check
> 
> 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