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: > > 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