From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-lf1-f48.google.com (mail-lf1-f48.google.com [209.85.167.48]) by mx.groups.io with SMTP id smtpd.web10.40948.1670845489274419238 for ; Mon, 12 Dec 2022 03:44:49 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@gmail.com header.s=20210112 header.b=AzmzTpKN; spf=pass (domain: gmail.com, ip: 209.85.167.48, mailfrom: savvamtr@gmail.com) Received: by mail-lf1-f48.google.com with SMTP id z26so205358lfu.8 for ; Mon, 12 Dec 2022 03:44:48 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=references:to:cc:in-reply-to:date:subject:mime-version:message-id :from:from:to:cc:subject:date:message-id:reply-to; bh=yM0ZXrKlkyJOdbysZ4GEAWgIbUx3slbVeD5lezpQ1G4=; b=AzmzTpKN0YHlfBsKulQKiN2ZTHUpfKFds3oOe431oH9yOKDBolzWTYZwaskbaCNOLG l9JEJRXG3VvsU4XRTfhxzHyR09d4hCC4PMXBjcmg84NsE2njM6dBCD5H77GHbNSVeECd csd7Tmyec8MsB1z/RoFNceYpqpxANIOUqqR+dbzq7fwu3gPS9vQuEtE66QGgTgyJ//MB 15iPCAmHNnVl+YtNB7GMhTp07LHf3gTKmiW8S77D3hhncUWybzMxqXtqYYuNJsl7W3ui OXG9ZGGq8JROdGEFlbgL2fS/3bv3MB0VrgCGjBhGkgyAIakGS5thP0uXijwcwo22CUFA Quiw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=references:to:cc:in-reply-to:date:subject:mime-version:message-id :from:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=yM0ZXrKlkyJOdbysZ4GEAWgIbUx3slbVeD5lezpQ1G4=; b=T9hdse0KSRQerHMKClsvqQkIH6flPvJTMhOuofLpER+zia7/2rm75SpCSiUBdhDKbd PC9ZuwKBmAvkyOd6utTuk4vMdsi3gs0j74n5yrMQ2PL8LVBRUKoZ0rkI8Z09A10YuNOc yDWgGlqG5YyYKYK68I46m2FUI9PpvhlGQF6P9nS+P54K99DOrGeeg31DVcaa9c3uMWcx YNb4gQUPkhiJBOOdnxrQ39cnYbqcNHETvPVcvsrUg8fqsZcagQekfc3oH5wvyz6lIfUd HK61eeL9stqjHHzZizQXViQKXGm+hujZXKMMtgVcz3relA/atZ6uy4aIenW12Q/7mtqO SmJw== X-Gm-Message-State: ANoB5pk8UV9cfA2s/0frM6duYooJwRin8dAPvRDL5PsxUampRF53nD+F NT8EEJT8zAhaYCXv9DiC1rg= X-Google-Smtp-Source: AA0mqf6Udl4uxlwVf3UcN3Pb31WxdqyRJWnk2s5AaPUXt6Of8SS4fqTIb890q2NFKRLSS9h/bzyptA== X-Received: by 2002:ac2:5fcc:0:b0:4a7:5a63:71ea with SMTP id q12-20020ac25fcc000000b004a75a6371eamr3684141lfg.15.1670845487175; Mon, 12 Dec 2022 03:44:47 -0800 (PST) Return-Path: Received: from smtpclient.apple (blg.theincredible.dev. [185.82.218.54]) by smtp.gmail.com with ESMTPSA id q21-20020a056512211500b004a26ba3458fsm1597045lfr.62.2022.12.12.03.44.45 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Mon, 12 Dec 2022 03:44:46 -0800 (PST) From: "Savva Mitrofanov" Message-Id: <53A8A05D-CC84-4AFE-AE86-759FAEDE96A3@gmail.com> Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3696.120.41.1.1\)) Subject: Re: [edk2-platforms][PATCH v1 06/12] Ext4Pkg: Add comparison between Position and FileSize in Ext4SetPosition Date: Mon, 12 Dec 2022 17:44:42 +0600 In-Reply-To: Cc: devel@edk2.groups.io, mhaeuser@posteo.de, =?utf-8?B?0JLQuNGC0LDQu9C40Lkg0K7RgNGM0LXQstC40Ycg0KfQtdC/0YbQvtCy?= To: Pedro Falcato References: <20221209161104.70220-1-savvamtr@gmail.com> <20221209161104.70220-7-savvamtr@gmail.com> X-Mailer: Apple Mail (2.3696.120.41.1.1) Content-Type: multipart/alternative; boundary="Apple-Mail=_FDA36577-D594-4CE5-9F71-C7DBF4A1CE7A" --Apple-Mail=_FDA36577-D594-4CE5-9F71-C7DBF4A1CE7A Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=us-ascii Seems I misunderstood the usage of SetPosition, thanks for pointing out. = So we can just drop this commit and keep everything as is, because this check is already present in = Ext4Read. Savva Mitrofanov > On 10 Dec 2022, at 04:12, Pedro Falcato = wrote: >=20 > On Fri, Dec 9, 2022 at 4:11 PM Savva Mitrofanov > wrote: > Missing such comparison leads to infinite loop states, for example = code > which trying to read entire file can easily get out of bound of > file size by passing position value which exceeds file size without = this > check. So we need to add there missing comparison between the desired > position to be set and file size >=20 > + FileSize =3D EXT4_INODE_SIZE (File->Inode); > + > // -1 (0xffffff.......) seeks to the end of the file > if (Position =3D=3D (UINT64)-1) { > - Position =3D EXT4_INODE_SIZE (File->Inode); > + Position =3D FileSize; > + } else if (Position > FileSize) { > + DEBUG ((DEBUG_FS, "[ext4] Ext4SetPosition Cannot seek to #%Lx of = %Lx\n", Position, FileSize)); > + return EFI_UNSUPPORTED; > } >=20 > File->Position =3D Position; >=20 > On further inspection, this case is covered in the UEFI spec. >=20 > = https://uefi.org/specs/UEFI/2.10/13_Protocols_Media_Access.html#efi-file-p= rotocol-read = says: >=20 > > EFI_DEVICE_ERROR On entry, the current file position is beyond = the end of the file. >=20 > while the standard does not say SetPosition() can error out for bad = seeks. >=20 > So I think we should allow this in SetPosition() and error out in = Read(). Does this look good to you? > =20 > Pedro --Apple-Mail=_FDA36577-D594-4CE5-9F71-C7DBF4A1CE7A Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=us-ascii
Seems I misunderstood the usage of SetPosition, thanks for = pointing out.  So we can just drop this commit
and keep everything as is, because this check is already = present in Ext4Read.

Savva Mitrofanov

On 10 = Dec 2022, at 04:12, Pedro Falcato <pedro.falcato@gmail.com> wrote:

On Fri, Dec 9, 2022 at 4:11 PM = Savva Mitrofanov <savvamtr@gmail.com> wrote:
Missing such comparison leads to = infinite loop states, for example code
which trying to read entire file can easily get out of bound of
file size by passing position value which exceeds file size without = this
check. So we need to add there missing comparison between the desired
position to be set and file size

+  FileSize =3D EXT4_INODE_SIZE (File->Inode);
+
   // -1 (0xffffff.......) seeks to the end of the file
   if (Position =3D=3D (UINT64)-1) {
-    Position =3D EXT4_INODE_SIZE (File->Inode);
+    Position =3D FileSize;
+  } else if (Position > FileSize) {
+    DEBUG ((DEBUG_FS, "[ext4] Ext4SetPosition Cannot seek to = #%Lx of %Lx\n", Position, FileSize));
+    return EFI_UNSUPPORTED;
   }

   File->Position =3D Position;

On further inspection, this case is covered in the UEFI = spec.

https://uefi.org/specs/UEFI/2.10/13_Protocols_Media_Access.html= #efi-file-protocol-read says:

> = EFI_DEVICE_ERROR     On entry, the current file = position is beyond the end of the file.

while the standard does not say = SetPosition() can error out for bad seeks.

So I think we should = allow this in SetPosition() and error out in Read(). Does this look good = to you?
 
Pedro

= --Apple-Mail=_FDA36577-D594-4CE5-9F71-C7DBF4A1CE7A--