From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by mx.groups.io with SMTP id smtpd.web11.8838.1610028987307955677 for ; Thu, 07 Jan 2021 06:16:28 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=UEcApHbo; spf=pass (domain: redhat.com, ip: 216.205.24.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1610028986; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=0ljpcY8EtSkmD/oggEftIqGiCY6LTUIvz6AgxI3mS/0=; b=UEcApHboBD2P6DxVg4e+FaGyFy3+M+dROI59nWW/MlcW1bWvU0W8r9xaqNwA3cPOjFW2FS wXfCVyh0MhfmFOGa4KAC8kQ/GrCs1da963q6NZ1GaM1hfXaPTcYXb9FORGiYh9HTbh4aT6 lD4UwFi94nckIBWdAlUrwpNxx/zdem8= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-309-8mwiegpDMt-enE0n4wAJaw-1; Thu, 07 Jan 2021 09:16:23 -0500 X-MC-Unique: 8mwiegpDMt-enE0n4wAJaw-1 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 393CE87950C; Thu, 7 Jan 2021 14:16:22 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-114-119.ams2.redhat.com [10.36.114.119]) by smtp.corp.redhat.com (Postfix) with ESMTP id 08A5260CE6; Thu, 7 Jan 2021 14:16:20 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH] OvmfPkg/VirtioFsDxe: call IsTimeValid() before EfiTimeToEpoch() To: devel@edk2.groups.io, ard.biesheuvel@arm.com Cc: Jordan Justen , =?UTF-8?Q?Philippe_Mathieu-Daud=c3=a9?= References: <20210107095051.22715-1-lersek@redhat.com> <54be7422-3c3a-255e-96f7-6b03313ec873@arm.com> From: "Laszlo Ersek" Message-ID: <4933644c-2bfc-ab30-4eef-f4e0995b029e@redhat.com> Date: Thu, 7 Jan 2021 15:16:20 +0100 MIME-Version: 1.0 In-Reply-To: <54be7422-3c3a-255e-96f7-6b03313ec873@arm.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=lersek@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit On 01/07/21 10:52, Ard Biesheuvel wrote: > On 1/7/21 10:50 AM, Laszlo Ersek wrote: >> EmbeddedPkg/TimeBaseLib provides a verification function called >> IsTimeValid(), for enforcing the UEFI spec requirements on an EFI_TIME >> object. >> >> When EFI_FILE_PROTOCOL.SetInfo() is called in order to update the >> timestamps on the file, let's invoke IsTimeValid() first, before passing >> the new EFI_FILE_INFO.{CreateTime,LastAccessTime,ModificationTime} values >> to EfiTimeToEpoch(). >> >> This patch is not expected to make a practical difference, but it's better >> to ascertain the preconditions of EfiTimeToEpoch() on the >> EFI_FILE_PROTOCOL.SetInfo() caller. The FAT driver (EnhancedFatDxe) has a >> similar check, namely in FatSetFileInfo() -> FatIsValidTime(). >> >> Cc: Ard Biesheuvel >> Cc: Jordan Justen >> Cc: Philippe Mathieu-Daudé >> Signed-off-by: Laszlo Ersek > > Acked-by: Ard Biesheuvel Merged via , as commit e9c5ff3d2730. Thanks! Laszlo > > >> --- >> OvmfPkg/VirtioFsDxe/Helpers.c | 20 +++++++++++++++----- >> 1 file changed, 15 insertions(+), 5 deletions(-) >> >> diff --git a/OvmfPkg/VirtioFsDxe/Helpers.c b/OvmfPkg/VirtioFsDxe/Helpers.c >> index 443bbdc616ac..b81c04e0a4e8 100644 >> --- a/OvmfPkg/VirtioFsDxe/Helpers.c >> +++ b/OvmfPkg/VirtioFsDxe/Helpers.c >> @@ -2237,26 +2237,33 @@ VirtioFsGetFuseSizeUpdate ( >> >> @param[out] Atime If UpdateAtime is set to TRUE, then Atime provides >> the last access timestamp to set (as seconds since >> the Epoch). Otherwise, Atime is not written to. >> >> @param[out] Mtime If UpdateMtime is set to TRUE, then Mtime provides >> the last modification timestamp to set (as seconds >> since the Epoch). Otherwise, Mtime is not written >> to. >> >> - @retval EFI_SUCCESS Output parameters have been set successfully. >> + @retval EFI_SUCCESS Output parameters have been set successfully. >> >> - @retval EFI_ACCESS_DENIED NewInfo requests changing both CreateTime and >> - ModificationTime, but to values that differ from >> - each other. The Virtio Filesystem device does not >> - support this. >> + @retval EFI_INVALID_PARAMETER At least one of the CreateTime, LastAccessTime >> + and ModificationTime fields in NewInfo >> + represents an actual update relative to the >> + current state of the file (expressed in Info), >> + but does not satisfy the UEFI spec >> + requirements on EFI_TIME. >> + >> + @retval EFI_ACCESS_DENIED NewInfo requests changing both CreateTime and >> + ModificationTime, but to values that differ >> + from each other. The Virtio Filesystem device >> + does not support this. >> **/ >> EFI_STATUS >> VirtioFsGetFuseTimeUpdates ( >> IN EFI_FILE_INFO *Info, >> IN EFI_FILE_INFO *NewInfo, >> OUT BOOLEAN *UpdateAtime, >> OUT BOOLEAN *UpdateMtime, >> OUT UINT64 *Atime, >> OUT UINT64 *Mtime >> ) >> @@ -2278,20 +2285,23 @@ VirtioFsGetFuseTimeUpdates ( >> // >> // Determine which timestamps differ from the current state. (A zero time >> // means "don't update", per UEFI spec.) For each timestamp that's being >> // changed, calculate the seconds since the Epoch. >> // >> for (Idx = 0; Idx < ARRAY_SIZE (Time); Idx++) { >> if (CompareMem (NewTime[Idx], &ZeroTime, sizeof (EFI_TIME)) == 0 || >> CompareMem (NewTime[Idx], Time[Idx], sizeof (EFI_TIME)) == 0) { >> Change[Idx] = FALSE; >> } else { >> + if (!IsTimeValid (NewTime[Idx])) { >> + return EFI_INVALID_PARAMETER; >> + } >> Change[Idx] = TRUE; >> Seconds[Idx] = EfiTimeToEpoch (NewTime[Idx]); >> } >> } >> >> // >> // If a change is requested for exactly one of CreateTime and >> // ModificationTime, we'll change the last modification time. If changes are >> // requested for both, and to the same timestamp, we'll similarly update the >> // last modification time. If changes are requested for both, but to >> > > > > > >