From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: redhat.com, ip: 209.85.128.68, mailfrom: philmd@redhat.com) Received: from mail-wm1-f68.google.com (mail-wm1-f68.google.com [209.85.128.68]) by groups.io with SMTP; Mon, 15 Apr 2019 10:24:33 -0700 Received: by mail-wm1-f68.google.com with SMTP id z11so21779828wmi.0 for ; Mon, 15 Apr 2019 10:24:32 -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:openpgp:message-id :date:user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=BLsxH26FZyTT3mUwcow5UXUkfvaSOcTVnFVKWLCtsb4=; b=QWemYuLhcoCRgFmOu/DHY66vu0NLsp2+xO2mRMTdc/9hyp4Gr5jkf3yfX3rxCw1hM0 TgYQb4OQhCBsnGNCAWsoEROUGXlgGlcWT155ocFsqrRZaxMogPugCnFzL9Q0Mre4TMyE NmTIuDwN7+/b/eHTGq0rnTmCsQV+b+ZKBdMiQ55X1YWHtW9T+uDU4tBPPM9DJghpmGp/ +9+T8dfj+C9dd31Wyd4iehUmhMDERRgdA90FKHPkGTsbUOYeibPM96zE8111wr2tDTlZ t8gQVbog09yd08zPUu5PKr5WNOW6hjZy+Im97PAW6Oy3kv0R2yrCkV9YE8257fiiin/S dEWg== X-Gm-Message-State: APjAAAUx5KT3dqs6RKL76q7K3PSyhRDxKrq7ZUvi/hGFsUoOIyzFNgoS YFpPet/Xsdx6/5f8Soj2YURqpQ== X-Google-Smtp-Source: APXvYqylkXzdDNC3zBKb/Bjj94rtri/Goxvx9uJapBGzk7k/7ZSRmGYPxfHPK74y7ZMsIguhY6zQkA== X-Received: by 2002:a7b:c155:: with SMTP id z21mr23273445wmi.1.1555349071497; Mon, 15 Apr 2019 10:24:31 -0700 (PDT) Return-Path: Received: from [10.32.224.40] (red-hat-inc.vlan560.asr1.mad1.gblx.net. [159.63.51.90]) by smtp.gmail.com with ESMTPSA id n4sm52507561wrx.39.2019.04.15.10.24.30 (version=TLS1_3 cipher=AEAD-AES128-GCM-SHA256 bits=128/128); Mon, 15 Apr 2019 10:24:30 -0700 (PDT) Subject: Re: [edk2-devel] [PATCH 05/10] OvmfPkg/Sec: fix out-of-bounds reads To: devel@edk2.groups.io, lersek@redhat.com Cc: Ard Biesheuvel , Jordan Justen References: <20190412233128.4756-1-lersek@redhat.com> <20190412233128.4756-6-lersek@redhat.com> From: =?UTF-8?B?UGhpbGlwcGUgTWF0aGlldS1EYXVkw6k=?= Openpgp: id=89C1E78F601EE86C867495CBA2A3FD6EDEADC0DE; url=http://pgp.mit.edu/pks/lookup?op=get&search=0xA2A3FD6EDEADC0DE Message-ID: <15bb06da-4c8f-e29f-e9ff-e5d3ecf568eb@redhat.com> Date: Mon, 15 Apr 2019 19:24:30 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.6.1 MIME-Version: 1.0 In-Reply-To: <20190412233128.4756-6-lersek@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 4/13/19 1:31 AM, Laszlo Ersek wrote: > RH covscan justifiedly reports that accessing "EFI_FFS_FILE_HEADER.Size" > and "EFI_COMMON_SECTION_HEADER.Size", which both are of type UINT8[3], > through (UINT32*), is undefined behavior: > >> Error: OVERRUN (CWE-119): >> edk2-89910a39dcfd/OvmfPkg/Sec/SecMain.c:283: overrun-local: Overrunning >> array of 3 bytes at byte offset 3 by dereferencing pointer >> "(UINT32 *)File->Size". >> # 281| >> # 282| File = (EFI_FFS_FILE_HEADER*)(UINTN) CurrentAddress; >> # 283|-> Size = *(UINT32*) File->Size & 0xffffff; >> # 284| if (Size < (sizeof (*File) + sizeof (EFI_COMMON_SECTION_HEADER))) { >> # 285| return EFI_VOLUME_CORRUPTED; >> >> Error: OVERRUN (CWE-119): >> edk2-89910a39dcfd/OvmfPkg/Sec/SecMain.c:614: overrun-local: Overrunning >> array of 3 bytes at byte offset 3 by dereferencing pointer >> "(UINT32 *)File->Size". >> # 612| >> # 613| File = (EFI_FFS_FILE_HEADER*)(UINTN) CurrentAddress; >> # 614|-> Size = *(UINT32*) File->Size & 0xffffff; >> # 615| if (Size < sizeof (*File)) { >> # 616| return EFI_NOT_FOUND; >> >> Error: OVERRUN (CWE-119): >> edk2-89910a39dcfd/OvmfPkg/Sec/SecMain.c:639: overrun-local: Overrunning >> array of 3 bytes at byte offset 3 by dereferencing pointer >> "(UINT32 *)Section->Size". >> # 637| Section = (EFI_COMMON_SECTION_HEADER*)(UINTN) CurrentAddress; >> # 638| >> # 639|-> Size = *(UINT32*) Section->Size & 0xffffff; >> # 640| if (Size < sizeof (*Section)) { >> # 641| return EFI_NOT_FOUND; > > Fix these by invoking the FFS_FILE_SIZE() and SECTION_SIZE() macros, which > by now have been fixed too. > > Cc: Ard Biesheuvel > Cc: Jordan Justen > Bugzilla: https://bugzilla.tianocore.org/show_bug.cgi?id=1710 > Issue: scan-1008.txt > Issue: scan-1009.txt > Issue: scan-1010.txt > Signed-off-by: Laszlo Ersek > --- > OvmfPkg/Sec/SecMain.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/OvmfPkg/Sec/SecMain.c b/OvmfPkg/Sec/SecMain.c > index 18a89c649fd4..3914355cd17b 100644 > --- a/OvmfPkg/Sec/SecMain.c > +++ b/OvmfPkg/Sec/SecMain.c > @@ -269,17 +269,17 @@ FindFfsFileAndSection ( > for (EndOfFile = CurrentAddress + Fv->HeaderLength; ; ) { > > CurrentAddress = (EndOfFile + 7) & ~(7ULL); > if (CurrentAddress > EndOfFirmwareVolume) { > return EFI_VOLUME_CORRUPTED; > } > > File = (EFI_FFS_FILE_HEADER*)(UINTN) CurrentAddress; > - Size = *(UINT32*) File->Size & 0xffffff; > + Size = FFS_FILE_SIZE (File); > if (Size < (sizeof (*File) + sizeof (EFI_COMMON_SECTION_HEADER))) { > return EFI_VOLUME_CORRUPTED; > } > > EndOfFile = CurrentAddress + Size; > if (EndOfFile > EndOfFirmwareVolume) { > return EFI_VOLUME_CORRUPTED; > } > @@ -600,17 +600,17 @@ FindImageBase ( > for (EndOfFile = CurrentAddress + BootFirmwareVolumePtr->HeaderLength; ; ) { > > CurrentAddress = (EndOfFile + 7) & 0xfffffffffffffff8ULL; > if (CurrentAddress > EndOfFirmwareVolume) { > return EFI_NOT_FOUND; > } > > File = (EFI_FFS_FILE_HEADER*)(UINTN) CurrentAddress; > - Size = *(UINT32*) File->Size & 0xffffff; > + Size = FFS_FILE_SIZE (File); > if (Size < sizeof (*File)) { > return EFI_NOT_FOUND; > } > > EndOfFile = CurrentAddress + Size; > if (EndOfFile > EndOfFirmwareVolume) { > return EFI_NOT_FOUND; > } > @@ -625,17 +625,17 @@ FindImageBase ( > // > // Loop through the FFS file sections within the FFS file > // > EndOfSection = (EFI_PHYSICAL_ADDRESS)(UINTN) (File + 1); > for (;;) { > CurrentAddress = (EndOfSection + 3) & 0xfffffffffffffffcULL; > Section = (EFI_COMMON_SECTION_HEADER*)(UINTN) CurrentAddress; > > - Size = *(UINT32*) Section->Size & 0xffffff; > + Size = SECTION_SIZE (Section); > if (Size < sizeof (*Section)) { > return EFI_NOT_FOUND; > } > > EndOfSection = CurrentAddress + Size; > if (EndOfSection > EndOfFile) { > return EFI_NOT_FOUND; > } > Reviewed-by: Philippe Mathieu-Daude