From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by mx.groups.io with SMTP id smtpd.web11.1663.1571045699517747262 for ; Mon, 14 Oct 2019 02:34:59 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: redhat.com, ip: 209.132.183.28, mailfrom: philmd@redhat.com) Received: from mail-wm1-f69.google.com (mail-wm1-f69.google.com [209.85.128.69]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id BF942550CF for ; Mon, 14 Oct 2019 09:34:58 +0000 (UTC) Received: by mail-wm1-f69.google.com with SMTP id r21so4067138wme.5 for ; Mon, 14 Oct 2019 02:34:58 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=Nw33h9f+cUCiJMyAic5yTmKm9/MDhnreRhtxfHwmROc=; b=oLTZDd95fbBrRNLMSV5z76qNnos62XzhjUqseGp2z0qqCQSwOUosF261QhDxsmHGdk O5G13Pjw3U2nVP6pGUcr4ZK5ou3QlC4uvBTAcCTEl513awMIMjqry6vaeAlNyzoPSIa4 iqF+8wiGT1Augd8BjU4YfyKk7DIbQu65qXYBY6lCAPzRorE2DRC6SYocCR2zLPBHWpko GKkzW0adKaNtbcUIxP7JbQAseHHqWMYiCKLT5nQ9RqxrHfX+x0lFxjQMHrBWdFAM72KK +QGN0cRpKL921z/lSO4KHLlJrnQsuzj9aZqw+hLRDF24F7iULT9oannScVx+bskpxCC+ WpLQ== X-Gm-Message-State: APjAAAVCmyW+qFs/0F8e8D/3rCovCZRMoZa7/mDRIADvvZlk84AmFOvB TgYoBnUUiTHVNQQPeQlkupRH82hBwcjNrzSfY6zWWMXlXXhyJSiVn0PeIZmEZ3eljqLqvRUZdM0 Ud2qtkREiV5yjnQ== X-Received: by 2002:a5d:43c1:: with SMTP id v1mr25669082wrr.91.1571045697468; Mon, 14 Oct 2019 02:34:57 -0700 (PDT) X-Google-Smtp-Source: APXvYqxBLAGFM/pChANhKBaynz7p5htGq8UxOYdaYHWCj8WRG+11+6vn41djF99V2ziCXLa9+l92xQ== X-Received: by 2002:a5d:43c1:: with SMTP id v1mr25669047wrr.91.1571045697093; Mon, 14 Oct 2019 02:34:57 -0700 (PDT) Received: from [192.168.50.32] (243.red-88-26-246.staticip.rima-tde.net. [88.26.246.243]) by smtp.gmail.com with ESMTPSA id r2sm28387849wrm.3.2019.10.14.02.34.56 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 14 Oct 2019 02:34:56 -0700 (PDT) Subject: Re: [edk2-devel] [PATCH v2] NetworkPkg/IScsiDxe: Fix the index of array TargetUrlp[] To: devel@edk2.groups.io, shenglei.zhang@intel.com Cc: Siyuan Fu , Jiaxin Wu , Laszlo Ersek References: <20191014031432.15984-1-shenglei.zhang@intel.com> From: =?UTF-8?B?UGhpbGlwcGUgTWF0aGlldS1EYXVkw6k=?= Message-ID: Date: Mon, 14 Oct 2019 11:34:55 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.1.0 MIME-Version: 1.0 In-Reply-To: <20191014031432.15984-1-shenglei.zhang@intel.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Hi Zhang, On 10/14/19 5:14 AM, Zhang, Shenglei wrote: > After the expression, > 'CopyMem (&ConfigNvData->TargetUrl, Field->Str, Field->Len);', > the '\0' should be set to TargetUrl[Field->Len] rather than > TargetUrl[Field->Len + 1]. ^ This is one change, ... > Besides the boundary check should be more strict. > Field->Len should range from 0-254 rather than 0-255. ^ ... and here we have another change. Can you split this in 2 patches? > Cc: Siyuan Fu > Cc: Jiaxin Wu > Signed-off-by: Shenglei Zhang > --- > > v2: Add missing ')' which causes build failure. > > NetworkPkg/IScsiDxe/IScsiDhcp.c | 7 ++++--- > NetworkPkg/IScsiDxe/IScsiDhcp6.c | 7 ++++--- > 2 files changed, 8 insertions(+), 6 deletions(-) > > diff --git a/NetworkPkg/IScsiDxe/IScsiDhcp.c b/NetworkPkg/IScsiDxe/IScsiDhcp.c > index d8c9fff6c65d..eac5b39991b7 100644 > --- a/NetworkPkg/IScsiDxe/IScsiDhcp.c > +++ b/NetworkPkg/IScsiDxe/IScsiDhcp.c > @@ -122,11 +122,12 @@ IScsiDhcpExtractRootPath ( > // > if ((!NET_IS_DIGIT (*(Field->Str))) && (*(Field->Str) != '[')) { > ConfigNvData->DnsMode = TRUE; > - if (Field->Len > sizeof (ConfigNvData->TargetUrl)) { > - return EFI_INVALID_PARAMETER; > + if (Field->Len >= sizeof (ConfigNvData->TargetUrl)) { > + Status = EFI_INVALID_PARAMETER; > + goto ON_EXIT; This is a change in the code flow. So now we free he allocated TmpStr. This is correct, but you did not commented that change in the description. So we currently have a memory leak. Please do not hide that kind of information in patches. > } > CopyMem (&ConfigNvData->TargetUrl, Field->Str, Field->Len); > - ConfigNvData->TargetUrl[Field->Len + 1] = '\0'; > + ConfigNvData->TargetUrl[Field->Len] = '\0'; And here we have 1 byte of memory info leak. Are you fixing a Security BZ? > } else { > ConfigNvData->DnsMode = FALSE; > ZeroMem(ConfigNvData->TargetUrl, sizeof (ConfigNvData->TargetUrl)); > diff --git a/NetworkPkg/IScsiDxe/IScsiDhcp6.c b/NetworkPkg/IScsiDxe/IScsiDhcp6.c > index 86a872adeccc..be66e6684a0e 100644 > --- a/NetworkPkg/IScsiDxe/IScsiDhcp6.c > +++ b/NetworkPkg/IScsiDxe/IScsiDhcp6.c > @@ -161,11 +161,12 @@ IScsiDhcp6ExtractRootPath ( > // Server name is expressed as domain name, just save it. > // > if (ConfigNvData->DnsMode) { > - if (Field->Len > sizeof (ConfigNvData->TargetUrl)) { > - return EFI_INVALID_PARAMETER; > + if (Field->Len >= sizeof (ConfigNvData->TargetUrl)) { > + Status = EFI_INVALID_PARAMETER; > + goto ON_EXIT; > } > CopyMem (&ConfigNvData->TargetUrl, Field->Str, Field->Len); > - ConfigNvData->TargetUrl[Field->Len + 1] = '\0'; > + ConfigNvData->TargetUrl[Field->Len] = '\0'; > } else { > ZeroMem(&ConfigNvData->TargetUrl, sizeof (ConfigNvData->TargetUrl)); > Status = IScsiAsciiStrToIp (Field->Str, IpMode, &Ip); >