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

["Fu, Siyuan" <siyuan.fu@intel.com>]

Hi, Jiaxin

I have 2 comments for the patch.
1. I think we can add the CNAME record to the DNS cache, just like the A and AAAA records, so the driver won't need to repeat the query if user query the host again.
2. We should hold the original query packet until the response is parsed successfully, otherwise the system will crash again if a retransmit is needed.

Best Regards
Siyuan


> -----Original Message-----
> From: Wu, Jiaxin
> Sent: Tuesday, September 6, 2016 11:39 AM
> To: edk2-devel@lists.01.org
> Cc: Hegde Nagaraj P <nagaraj-p.hegde@hpe.com>; Fu, Siyuan
> <siyuan.fu@intel.com>; Ye, Ting <ting.ye@intel.com>
> Subject: [Patch] NetworkPkg/DnsDxe: Handle CNAME type responded from the
> name server
> 
> 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 | 60 ++++++++++++++++++++++++++--------------
> -----
>  1 file changed, 35 insertions(+), 25 deletions(-)
> 
> diff --git a/NetworkPkg/DnsDxe/DnsImpl.c b/NetworkPkg/DnsDxe/DnsImpl.c
> index 360f68e..c68ec88 100644
> --- a/NetworkPkg/DnsDxe/DnsImpl.c
> +++ b/NetworkPkg/DnsDxe/DnsImpl.c
> @@ -1231,17 +1231,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 +1281,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 +1343,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 +1422,31 @@ 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;
> 
>          UpdateDns4Cache (&mDriverData->Dns4CacheList, FALSE, TRUE,
> *Dns4CacheEntry);
> 
> -        IpCount ++;
> +        IpCount ++;
> +        Status = EFI_SUCCESS;
>          break;
>        case DNS_TYPE_AAAA:
>          //
>          // This is address entry, get Data.
>          //
> @@ -1476,30 +1474,38 @@ 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;
> 
>          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, let's skip this CNAME record parsing and
> continue the next one.
> +        //
>          break;
>        default:
>          Status = EFI_UNSUPPORTED;
>          goto ON_EXIT;
>        }
> @@ -1539,24 +1545,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 ();
>      }
>    }
> --
> 1.9.5.msysgit.1