From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pf1-f180.google.com (mail-pf1-f180.google.com [209.85.210.180]) by mx.groups.io with SMTP id smtpd.web11.1004.1670623958886423835 for ; Fri, 09 Dec 2022 14:12:38 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@gmail.com header.s=20210112 header.b=e8BqB2Ba; spf=pass (domain: gmail.com, ip: 209.85.210.180, mailfrom: pedro.falcato@gmail.com) Received: by mail-pf1-f180.google.com with SMTP id c13so4668971pfp.5 for ; Fri, 09 Dec 2022 14:12:38 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=PX5jPG9wPS88tAjxvMHJRblQ/Tz1PbQXQwAQM0y+JdI=; b=e8BqB2BaC9fAdR2qBixbU4dGDwH7L/oLQdtfiA1qfU821wQWWCgVzUvffztj7knLVL XknEf71ZaMbvieEKmN+uzje9dIgYQPVWR0bLAzxWYwrHDdfAiY0mQhQ1xVqoCPHblhrp rUJWTMLtpx6DKMPiTvDJgz7gtzBsVn41bNsYHaiO9+yWqmrW68vaPlKMn50oUr96Mlog VUx3NUcxfLwlo32R0GCzeAe3cgE0tn0WHEFJGy84UybdKyPUWcxln4TwY0/aLlGphra5 8/l4ABDr+8YAHSNybyFPekXMQ/tB/zFKz+Zc5/jEyH9MZaou00XzYredOuUrkSPPOPvz EUpg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=PX5jPG9wPS88tAjxvMHJRblQ/Tz1PbQXQwAQM0y+JdI=; b=FrOcnTVtuW2dSe9aFP+wpy/bTqwTjGumMtqXs5a9xnsU9st4olAOV6LAhBHDdNu7Mm FyS7Kc1ZyE2lXTYhvjC9CbF3L6unR3xSq/vKs5KWP2H/4yMqrV1VIUyKFDh9bajuB8pF erLS8s4U7NaXoAXapUKSft2dCgpKDvzoxwFvPjq2NcZ1MkCbWiaksrnEIIzhgU23+WB+ lulkY2l0kIrbllkCxxXfzxJOaOS7Ab8SsvF+bzX85DvrMecLopikAdBKjGogc9u5iCHb 7Ck8lgjkP/xNpCEI4HzzdfLO9OfBkFtL3wY1CrFH2CYGmycv86fqCsg6Gw7Exx6s84lu 75NQ== X-Gm-Message-State: ANoB5plT6a81Zs8trzCqTLVEcubgUvAuuMIAWu0ecLU1/Lxf9EoSYwDw WtDY9z35cuBUd0TOFnSZdVogKLWKwkrw8UIMtGw= X-Google-Smtp-Source: AA0mqf6Q0noST4nbbt9iJwKtRBd+XPBtzwH4lBTB/oq11efMLIOS1ICKT7PiH7ufZ3Hrnnxsmf8IX1kcbon2hlMwh0M= X-Received: by 2002:a63:1206:0:b0:43c:76f4:c666 with SMTP id h6-20020a631206000000b0043c76f4c666mr74063631pgl.90.1670623958267; Fri, 09 Dec 2022 14:12:38 -0800 (PST) MIME-Version: 1.0 References: <20221209161104.70220-1-savvamtr@gmail.com> <20221209161104.70220-7-savvamtr@gmail.com> In-Reply-To: <20221209161104.70220-7-savvamtr@gmail.com> From: "Pedro Falcato" Date: Fri, 9 Dec 2022 22:12:27 +0000 Message-ID: Subject: Re: [edk2-platforms][PATCH v1 06/12] Ext4Pkg: Add comparison between Position and FileSize in Ext4SetPosition To: Savva Mitrofanov Cc: devel@edk2.groups.io, =?UTF-8?Q?Marvin_H=C3=A4user?= , Vitaly Cheptsov Content-Type: multipart/alternative; boundary="00000000000095fccd05ef6c735c" --00000000000095fccd05ef6c735c Content-Type: text/plain; charset="UTF-8" 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 > > + FileSize = EXT4_INODE_SIZE (File->Inode); > + > // -1 (0xffffff.......) seeks to the end of the file > if (Position == (UINT64)-1) { > - Position = EXT4_INODE_SIZE (File->Inode); > + Position = FileSize; > + } else if (Position > FileSize) { > + DEBUG ((DEBUG_FS, "[ext4] Ext4SetPosition Cannot seek to #%Lx of > %Lx\n", Position, FileSize)); > + return EFI_UNSUPPORTED; > } > > File->Position = 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 --00000000000095fccd05ef6c735c Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
On Fri, Dec 9, 2022 at 4:11 PM Savva Mitr= ofanov <savvamtr@gmail.com>= wrote:
Missing such comparison leads to infinite loop states, for e= xample 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

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

=C2=A0 =C2=A0File->Position =3D Position;
=

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


> EFI_DEVICE_ERROR=C2=A0=C2=A0=C2=A0=C2=A0 O= n 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 thi= s in SetPosition() and error out in Read(). Does this look good to you?
=C2=A0
Pedro
--00000000000095fccd05ef6c735c--