Re: [PATCH v2] NetworkPkg/DnsDxe: Handle CNAME type responded from the name server

["Hegde, Nagaraj P" <nagaraj-p.hegde@hpe.com>]

Tested-by: Hegde Nagaraj P <nagaraj-p.hegde@hpe.com>

-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Fu, Siyuan
Sent: Tuesday, September 06, 2016 2:37 PM
To: Wu, Jiaxin <jiaxin.wu@intel.com>; edk2-devel@lists.01.org
Cc: Ye, Ting <ting.ye@intel.com>
Subject: Re: [edk2] [PATCH v2] NetworkPkg/DnsDxe: Handle CNAME type responded from the name server

Reviewed-by: Fu Siyuan <siyuan.fu@intel.com>


> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of 
> Jiaxin Wu
> Sent: Tuesday, September 6, 2016 4:51 PM
> To: edk2-devel@lists.01.org
> Cc: Ye, Ting <ting.ye@intel.com>; Fu, Siyuan <siyuan.fu@intel.com>
> Subject: [edk2] [PATCH v2] NetworkPkg/DnsDxe: Handle CNAME type 
> responded from the name server
> 
> v2:
> * Code refine.
> * For DnsCache, the minimum value of TTL is selected between CNAME and 
> A/AAAA record.
> 
> According RFC 1034 - 3.6.2, if the query name is an alias, the name 
> server will include the CNAME record in the response and restart the 
> query at the domain name specified in the data field of the CNAME 
> record. RFC also provides one example server action when A query 
> received:
> 
> Suppose a name server was processing a query with for USCISIC.ARPA, 
> asking for type A information, and had the following resource records:
> USC-ISIC.ARPA IN CNAME C.ISI.EDU
> C.ISI.EDU     IN A     10.0.0.52
> Both of these RRs would be returned in the response to the type A query.
> 
> Currently, DnsDxe driver doesn't handle the CNAME type response, which 
> will cause any exception result. The driver need continue the packet 
> parsing while CNAME type record parsed. So, this patch is used to 
> handle it correctly.
> 
> Cc: Hegde Nagaraj P <nagaraj-p.hegde@hpe.com>
> Cc: Fu Siyuan <siyuan.fu@intel.com>
> Cc: Ye Ting <ting.ye@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Jiaxin Wu <jiaxin.wu@intel.com>
> ---
>  NetworkPkg/DnsDxe/DnsImpl.c | 81 
> +++++++++++++++++++++++++++++-----------
> -----
>  1 file changed, 52 insertions(+), 29 deletions(-)
> 
> diff --git a/NetworkPkg/DnsDxe/DnsImpl.c b/NetworkPkg/DnsDxe/DnsImpl.c 
> index 360f68e..cfaa4c7 100644
> --- a/NetworkPkg/DnsDxe/DnsImpl.c
> +++ b/NetworkPkg/DnsDxe/DnsImpl.c
> @@ -1125,10 +1125,11 @@ ParseDnsResponse (
>    DNS6_TOKEN_ENTRY      *Dns6TokenEntry;
> 
>    UINT32                IpCount;
>    UINT32                RRCount;
>    UINT32                AnswerSectionNum;
> +  UINT32                CNameTtl;
> 
>    EFI_IPv4_ADDRESS      *HostAddr4;
>    EFI_IPv6_ADDRESS      *HostAddr6;
> 
>    EFI_DNS4_CACHE_ENTRY  *Dns4CacheEntry; @@ -1146,10 +1147,11 @@ 
> ParseDnsResponse (
>    Dns6TokenEntry   = NULL;
> 
>    IpCount          = 0;
>    RRCount          = 0;
>    AnswerSectionNum = 0;
> +  CNameTtl         = 0;
> 
>    HostAddr4        = NULL;
>    HostAddr6        = NULL;
> 
>    Dns4CacheEntry   = NULL;
> @@ -1231,17 +1233,10 @@ ParseDnsResponse (
>    if (DnsHeader->Flags.Bits.RCode != DNS_FLAGS_RCODE_NO_ERROR ||
> DnsHeader->AnswersNum < 1 || \
>        DnsHeader->Flags.Bits.QR != DNS_FLAGS_QR_RESPONSE) {
>        Status = EFI_ABORTED;
>        goto ON_EXIT;
>    }
> -
> -  //
> -  // Free the sending packet.
> -  //
> -  if (Item->Value != NULL) {
> -    NetbufFree ((NET_BUF *) (Item->Value));
> -  }
> 
>    //
>    // Do some buffer allocations.
>    //
>    if (Instance->Service->IpVersion == IP_VERSION_4) { @@ -1288,40 
> +1283,42 @@ ParseDnsResponse (
>        //
>        // It's the GeneralLookUp querying.
>        //
>        Dns6TokenEntry->Token->RspData.GLookupData = AllocatePool 
> (sizeof (DNS_RESOURCE_RECORD));
>        if (Dns6TokenEntry->Token->RspData.GLookupData == NULL) {
> -        Status = EFI_UNSUPPORTED;
> +        Status = EFI_OUT_OF_RESOURCES;
>          goto ON_EXIT;
>        }
>        Dns6TokenEntry->Token->RspData.GLookupData->RRList = 
> AllocatePool (DnsHeader->AnswersNum * sizeof (DNS_RESOURCE_RECORD));
>        if (Dns6TokenEntry->Token->RspData.GLookupData->RRList == NULL) {
> -        Status = EFI_UNSUPPORTED;
> +        Status = EFI_OUT_OF_RESOURCES;
>          goto ON_EXIT;
>        }
>      } else {
>        //
>        // It's not the GeneralLookUp querying. Check the Query type.
>        //
>        if (QuerySection->Type == DNS_TYPE_AAAA) {
>          Dns6TokenEntry->Token->RspData.H2AData = AllocatePool (sizeof 
> (DNS6_HOST_TO_ADDR_DATA));
>          if (Dns6TokenEntry->Token->RspData.H2AData == NULL) {
> -          Status = EFI_UNSUPPORTED;
> +          Status = EFI_OUT_OF_RESOURCES;
>            goto ON_EXIT;
>          }
>          Dns6TokenEntry->Token->RspData.H2AData->IpList = AllocatePool 
> (DnsHeader->AnswersNum * sizeof (EFI_IPv6_ADDRESS));
>          if (Dns6TokenEntry->Token->RspData.H2AData->IpList == NULL) {
> -          Status = EFI_UNSUPPORTED;
> +          Status = EFI_OUT_OF_RESOURCES;
>            goto ON_EXIT;
>          }
>        } else {
>          Status = EFI_UNSUPPORTED;
>          goto ON_EXIT;
>        }
>      }
>    }
> 
> +  Status = EFI_NOT_FOUND;
> +
>    //
>    // Processing AnswerSection.
>    //
>    while (AnswerSectionNum < DnsHeader->AnswersNum) {
>      //
> @@ -1348,51 +1345,53 @@ ParseDnsResponse (
>        //
>        // Fill the ResourceRecord.
>        //
>        Dns4RR[RRCount].QName = AllocateZeroPool (AsciiStrLen 
> (QueryName) + 1);
>        if (Dns4RR[RRCount].QName == NULL) {
> -        Status = EFI_UNSUPPORTED;
> +        Status = EFI_OUT_OF_RESOURCES;
>          goto ON_EXIT;
>        }
>        CopyMem (Dns4RR[RRCount].QName, QueryName, AsciiStrLen (QueryName));
>        Dns4RR[RRCount].QType = AnswerSection->Type;
>        Dns4RR[RRCount].QClass = AnswerSection->Class;
>        Dns4RR[RRCount].TTL = AnswerSection->Ttl;
>        Dns4RR[RRCount].DataLength = AnswerSection->DataLength;
>        Dns4RR[RRCount].RData = AllocateZeroPool 
> (Dns4RR[RRCount].DataLength);
>        if (Dns4RR[RRCount].RData == NULL) {
> -        Status = EFI_UNSUPPORTED;
> +        Status = EFI_OUT_OF_RESOURCES;
>          goto ON_EXIT;
>        }
>        CopyMem (Dns4RR[RRCount].RData, AnswerData, 
> Dns4RR[RRCount].DataLength);
> 
>        RRCount ++;
> +      Status = EFI_SUCCESS;
>      } else if (Instance->Service->IpVersion == IP_VERSION_6 &&
> Dns6TokenEntry->GeneralLookUp) {
>        Dns6RR = Dns6TokenEntry->Token->RspData.GLookupData->RRList;
>        AnswerData = (UINT8 *) AnswerSection + sizeof (*AnswerSection);
> 
>        //
>        // Fill the ResourceRecord.
>        //
>        Dns6RR[RRCount].QName = AllocateZeroPool (AsciiStrLen 
> (QueryName) + 1);
>        if (Dns6RR[RRCount].QName == NULL) {
> -        Status = EFI_UNSUPPORTED;
> +        Status = EFI_OUT_OF_RESOURCES;
>          goto ON_EXIT;
>        }
>        CopyMem (Dns6RR[RRCount].QName, QueryName, AsciiStrLen (QueryName));
>        Dns6RR[RRCount].QType = AnswerSection->Type;
>        Dns6RR[RRCount].QClass = AnswerSection->Class;
>        Dns6RR[RRCount].TTL = AnswerSection->Ttl;
>        Dns6RR[RRCount].DataLength = AnswerSection->DataLength;
>        Dns6RR[RRCount].RData = AllocateZeroPool 
> (Dns6RR[RRCount].DataLength);
>        if (Dns6RR[RRCount].RData == NULL) {
> -        Status = EFI_UNSUPPORTED;
> +        Status = EFI_OUT_OF_RESOURCES;
>          goto ON_EXIT;
>        }
>        CopyMem (Dns6RR[RRCount].RData, AnswerData, 
> Dns6RR[RRCount].DataLength);
> 
>        RRCount ++;
> +      Status = EFI_SUCCESS;
>      } else {
>        //
>        // It's not the GeneralLookUp querying.
>        // Check the Query type, parse the response packet.
>        //
> @@ -1425,30 +1424,36 @@ ParseDnsResponse (
>          //
>          // Allocate new CacheEntry pool.
>          //
>          Dns4CacheEntry = AllocateZeroPool (sizeof (EFI_DNS4_CACHE_ENTRY));
>          if (Dns4CacheEntry == NULL) {
> -          Status = EFI_UNSUPPORTED;
> +          Status = EFI_OUT_OF_RESOURCES;
>            goto ON_EXIT;
>          }
>          Dns4CacheEntry->HostName = AllocateZeroPool (2 *
> (StrLen(Dns4TokenEntry->QueryHostName) + 1));
>          if (Dns4CacheEntry->HostName == NULL) {
> -          Status = EFI_UNSUPPORTED;
> +          Status = EFI_OUT_OF_RESOURCES;
>            goto ON_EXIT;
>          }
>          CopyMem (Dns4CacheEntry->HostName, 
> Dns4TokenEntry->QueryHostName,
> 2 * (StrLen(Dns4TokenEntry->QueryHostName) + 1));
>          Dns4CacheEntry->IpAddress = AllocateZeroPool (sizeof 
> (EFI_IPv4_ADDRESS));
>          if (Dns4CacheEntry->IpAddress == NULL) {
> -          Status = EFI_UNSUPPORTED;
> +          Status = EFI_OUT_OF_RESOURCES;
>            goto ON_EXIT;
>          }
>          CopyMem (Dns4CacheEntry->IpAddress, AnswerData, sizeof 
> (EFI_IPv4_ADDRESS));
> -        Dns4CacheEntry->Timeout = AnswerSection->Ttl;
> +
> +        if (CNameTtl != 0 && AnswerSection->Ttl != 0) {
> +          Dns4CacheEntry->Timeout = MIN (CNameTtl, AnswerSection->Ttl);
> +        } else {
> +          Dns4CacheEntry->Timeout = MAX (CNameTtl, AnswerSection->Ttl);
> +        }
> 
>          UpdateDns4Cache (&mDriverData->Dns4CacheList, FALSE, TRUE, 
> *Dns4CacheEntry);
> 
> -        IpCount ++;
> +        IpCount ++;
> +        Status = EFI_SUCCESS;
>          break;
>        case DNS_TYPE_AAAA:
>          //
>          // This is address entry, get Data.
>          //
> @@ -1476,30 +1481,44 @@ ParseDnsResponse (
>          //
>          // Allocate new CacheEntry pool.
>          //
>          Dns6CacheEntry = AllocateZeroPool (sizeof (EFI_DNS6_CACHE_ENTRY));
>          if (Dns6CacheEntry == NULL) {
> -          Status = EFI_UNSUPPORTED;
> +          Status = EFI_OUT_OF_RESOURCES;
>            goto ON_EXIT;
>          }
>          Dns6CacheEntry->HostName = AllocateZeroPool (2 *
> (StrLen(Dns6TokenEntry->QueryHostName) + 1));
>          if (Dns6CacheEntry->HostName == NULL) {
> -          Status = EFI_UNSUPPORTED;
> +          Status = EFI_OUT_OF_RESOURCES;
>            goto ON_EXIT;
>          }
>          CopyMem (Dns6CacheEntry->HostName, 
> Dns6TokenEntry->QueryHostName,
> 2 * (StrLen(Dns6TokenEntry->QueryHostName) + 1));
>          Dns6CacheEntry->IpAddress = AllocateZeroPool (sizeof 
> (EFI_IPv6_ADDRESS));
>          if (Dns6CacheEntry->IpAddress == NULL) {
> -          Status = EFI_UNSUPPORTED;
> +          Status = EFI_OUT_OF_RESOURCES;
>            goto ON_EXIT;
>          }
>          CopyMem (Dns6CacheEntry->IpAddress, AnswerData, sizeof 
> (EFI_IPv6_ADDRESS));
> -        Dns6CacheEntry->Timeout = AnswerSection->Ttl;
> +
> +        if (CNameTtl != 0 && AnswerSection->Ttl != 0) {
> +          Dns6CacheEntry->Timeout = MIN (CNameTtl, AnswerSection->Ttl);
> +        } else {
> +          Dns6CacheEntry->Timeout = MAX (CNameTtl, AnswerSection->Ttl);
> +        }
> 
>          UpdateDns6Cache (&mDriverData->Dns6CacheList, FALSE, TRUE, 
> *Dns6CacheEntry);
> 
>          IpCount ++;
> +        Status = EFI_SUCCESS;
> +        break;
> +      case DNS_TYPE_CNAME:
> +        //
> +        // According RFC 1034 - 3.6.2, if the query name is an alias, 
> + the
> name server will include the CNAME
> +        // record in the response and restart the query at the domain
> name specified in the data field of the
> +        // CNAME record. So, just record the TTL value of the CNAME, 
> + then
> skip to parse the next record.
> +        //
> +        CNameTtl = AnswerSection->Ttl;
>          break;
>        default:
>          Status = EFI_UNSUPPORTED;
>          goto ON_EXIT;
>        }
> @@ -1539,24 +1558,28 @@ ParseDnsResponse (
>        }
>      }
>    }
> 
>    //
> -  // Parsing is complete, SignalEvent here.
> +  // Parsing is complete, free the sending packet and signal Event here.
>    //
> +  if (Item != NULL && Item->Value != NULL) {
> +    NetbufFree ((NET_BUF *) (Item->Value));  }
> +
>    if (Instance->Service->IpVersion == IP_VERSION_4) {
>      ASSERT (Dns4TokenEntry != NULL);
>      Dns4RemoveTokenEntry (&Instance->Dns4TxTokens, Dns4TokenEntry);
> -    Dns4TokenEntry->Token->Status = EFI_SUCCESS;
> +    Dns4TokenEntry->Token->Status = Status;
>      if (Dns4TokenEntry->Token->Event != NULL) {
>        gBS->SignalEvent (Dns4TokenEntry->Token->Event);
>        DispatchDpc ();
>      }
>    } else {
>      ASSERT (Dns6TokenEntry != NULL);
>      Dns6RemoveTokenEntry (&Instance->Dns6TxTokens, Dns6TokenEntry);
> -    Dns6TokenEntry->Token->Status = EFI_SUCCESS;
> +    Dns6TokenEntry->Token->Status = Status;
>      if (Dns6TokenEntry->Token->Event != NULL) {
>        gBS->SignalEvent (Dns6TokenEntry->Token->Event);
>        DispatchDpc ();
>      }
>    }
> @@ -2035,11 +2058,11 @@ DnsOnTimerUpdate (
>    }
> 
>    Entry = mDriverData->Dns4CacheList.ForwardLink;
>    while (Entry != &mDriverData->Dns4CacheList) {
>      Item4 = NET_LIST_USER_STRUCT (Entry, DNS4_CACHE, AllCacheLink);
> -    if (Item4->DnsCache.Timeout<=0) {
> +    if (Item4->DnsCache.Timeout == 0) {
>        RemoveEntryList (&Item4->AllCacheLink);
>        Entry = mDriverData->Dns4CacheList.ForwardLink;
>      } else {
>        Entry = Entry->ForwardLink;
>      }
> @@ -2054,11 +2077,11 @@ DnsOnTimerUpdate (
>    }
> 
>    Entry = mDriverData->Dns6CacheList.ForwardLink;
>    while (Entry != &mDriverData->Dns6CacheList) {
>      Item6 = NET_LIST_USER_STRUCT (Entry, DNS6_CACHE, AllCacheLink);
> -    if (Item6->DnsCache.Timeout<=0) {
> +    if (Item6->DnsCache.Timeout == 0) {
>        RemoveEntryList (&Item6->AllCacheLink);
>        Entry = mDriverData->Dns6CacheList.ForwardLink;
>      } else {
>        Entry = Entry->ForwardLink;
>      }
> --
> 1.9.5.msysgit.1
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel