MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c | 9 +++++++-- MdeModulePkg/Universal/Disk/UdfDxe/File.c | 6 ++++-- MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c | 5 +++++ 3 files changed, 16 insertions(+), 4 deletions(-)
Repo: https://github.com/lersek/edk2.git
Branch: udf_fixes_cleanups
Patches #2, #3 and #4 are needed (and enough) for me to build OVMF for
IA32 and X64 with clang-3.8, after the UDF introduction.
Patches #1 and #5 are cleanups that I felt fit before patch #2 and after
patch #4, respectively.
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Paulo Alcantara <pcacjr@zytor.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Thanks
Laszlo
Laszlo Ersek (5):
MdeModulePkg/UdfDxe: ASSERT() valid ReadFileInfo Flags for INLINE_DATA
req
MdeModulePkg/UdfDxe: don't return unset Status if INLINE_DATA req
succeeds
MdeModulePkg/UdfDxe: replace zero-init of local variables with
ZeroMem()
MdeModulePkg/PartitionDxe: don't divide 64-bit values with C operators
MdeModulePkg/PartitionDxe: remove always false comparison
MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c | 9 +++++++--
MdeModulePkg/Universal/Disk/UdfDxe/File.c | 6 ++++--
MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c | 5 +++++
3 files changed, 16 insertions(+), 4 deletions(-)
--
2.14.1.3.gb7cf6e02401b
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Hi Laszlo, How could we configure the Qemu and test the UDF driver on OVMF? BTW, how could we configure the Qemu to create a full feature scope machine to include all possible devices in it, e.g. USB, ISA, SD/MMC, network etc. Steven Shi Intel\SSG\STO\UEFI Firmware Tel: +86 021-61166522 iNet: 821-6522 > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of > Laszlo Ersek > Sent: Sunday, September 10, 2017 8:13 AM > To: edk2-devel-01 <edk2-devel@lists.01.org> > Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Dong, Eric <eric.dong@intel.com>; Zeng, > Star <star.zeng@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org> > Subject: [edk2] [PATCH 0/5] MdeModulePkg: UDF fixes and cleanups > > Repo: https://github.com/lersek/edk2.git > Branch: udf_fixes_cleanups > > Patches #2, #3 and #4 are needed (and enough) for me to build OVMF for > IA32 and X64 with clang-3.8, after the UDF introduction. > > Patches #1 and #5 are cleanups that I felt fit before patch #2 and after > patch #4, respectively. > > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> > Cc: Eric Dong <eric.dong@intel.com> > Cc: Paulo Alcantara <pcacjr@zytor.com> > Cc: Ruiyu Ni <ruiyu.ni@intel.com> > Cc: Star Zeng <star.zeng@intel.com> > > Thanks > Laszlo > > Laszlo Ersek (5): > MdeModulePkg/UdfDxe: ASSERT() valid ReadFileInfo Flags for INLINE_DATA > req > MdeModulePkg/UdfDxe: don't return unset Status if INLINE_DATA req > succeeds > MdeModulePkg/UdfDxe: replace zero-init of local variables with > ZeroMem() > MdeModulePkg/PartitionDxe: don't divide 64-bit values with C operators > MdeModulePkg/PartitionDxe: remove always false comparison > > MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c | 9 +++++++-- > MdeModulePkg/Universal/Disk/UdfDxe/File.c | 6 ++++-- > MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c | 5 +++++ > 3 files changed, 16 insertions(+), 4 deletions(-) > > -- > 2.14.1.3.gb7cf6e02401b > > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 09/10/17 06:24, Shi, Steven wrote: > Hi Laszlo, > How could we configure the Qemu and test the UDF driver on OVMF? I guess you would format e.g. a DVD image with UDF, and attach it to QEMU like any other CD-ROM. > BTW, how could we configure the Qemu to create a full feature scope machine to include all possible devices in it, e.g. USB, ISA, SD/MMC, network etc. This question is impossible to answer, there are so many device models in QEMU. Instead, if you have a specific driver in edk2 that you would like to test, I'd recommend adding a device model to the machine configuration just for that. If you have several drivers in mind, repeat until happy. Either way, I'd certainly not recommend the raw QEMU command line for this; I recommend libvirtd, and virt-manager + "virsh edit". http://libvirt.org/formatdomain.html Thanks Laszlo > > > Steven Shi > Intel\SSG\STO\UEFI Firmware > > Tel: +86 021-61166522 > iNet: 821-6522 > >> -----Original Message----- >> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of >> Laszlo Ersek >> Sent: Sunday, September 10, 2017 8:13 AM >> To: edk2-devel-01 <edk2-devel@lists.01.org> >> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Dong, Eric <eric.dong@intel.com>; Zeng, >> Star <star.zeng@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org> >> Subject: [edk2] [PATCH 0/5] MdeModulePkg: UDF fixes and cleanups >> >> Repo: https://github.com/lersek/edk2.git >> Branch: udf_fixes_cleanups >> >> Patches #2, #3 and #4 are needed (and enough) for me to build OVMF for >> IA32 and X64 with clang-3.8, after the UDF introduction. >> >> Patches #1 and #5 are cleanups that I felt fit before patch #2 and after >> patch #4, respectively. >> >> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> >> Cc: Eric Dong <eric.dong@intel.com> >> Cc: Paulo Alcantara <pcacjr@zytor.com> >> Cc: Ruiyu Ni <ruiyu.ni@intel.com> >> Cc: Star Zeng <star.zeng@intel.com> >> >> Thanks >> Laszlo >> >> Laszlo Ersek (5): >> MdeModulePkg/UdfDxe: ASSERT() valid ReadFileInfo Flags for INLINE_DATA >> req >> MdeModulePkg/UdfDxe: don't return unset Status if INLINE_DATA req >> succeeds >> MdeModulePkg/UdfDxe: replace zero-init of local variables with >> ZeroMem() >> MdeModulePkg/PartitionDxe: don't divide 64-bit values with C operators >> MdeModulePkg/PartitionDxe: remove always false comparison >> >> MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c | 9 +++++++-- >> MdeModulePkg/Universal/Disk/UdfDxe/File.c | 6 ++++-- >> MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c | 5 +++++ >> 3 files changed, 16 insertions(+), 4 deletions(-) >> >> -- >> 2.14.1.3.gb7cf6e02401b >> >> _______________________________________________ >> edk2-devel mailing list >> edk2-devel@lists.01.org >> https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Hi Steven, On 09/10/17 10:38, Laszlo Ersek wrote: > On 09/10/17 06:24, Shi, Steven wrote: >> Hi Laszlo, >> How could we configure the Qemu and test the UDF driver on OVMF? > > I guess you would format e.g. a DVD image with UDF, and attach it to > QEMU like any other CD-ROM. I tried to look into this -- I tried several things, but nothing produced an UDF image file that, when attached to the VM, would show up in the UEFI shell as FSn: Google returned a bunch of pages, but all I found was: - tips that didn't work (see above), - confused users (like me) looking for solutions. So, at the moment, I have no idea how authoring UDF DVD images is possible on Linux, so that they'd be recognized in edk2. (I'm interested in the edk2 UDF driver not because I want to "author" UDF DVD images (ISO9660+ElTorito works just fine), but because some optical media images that were given to me are formatted UDF-only. They can be translated into ISO9660+ElTorito off-line, but that's a chore.) Thanks, Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
OK. Does the UDF image you created correctly show up as CD-ROM content in Linux, e.g Fedora? Steven Shi Intel\SSG\STO\UEFI Firmware Tel: +86 021-61166522 iNet: 821-6522 > -----Original Message----- > From: Laszlo Ersek [mailto:lersek@redhat.com] > Sent: Sunday, September 10, 2017 9:52 PM > To: Shi, Steven <steven.shi@intel.com>; edk2-devel-01 <edk2- > devel@lists.01.org> > Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Dong, Eric <eric.dong@intel.com>; Zeng, > Star <star.zeng@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org> > Subject: Re: [edk2] [PATCH 0/5] MdeModulePkg: UDF fixes and cleanups > > Hi Steven, > > On 09/10/17 10:38, Laszlo Ersek wrote: > > On 09/10/17 06:24, Shi, Steven wrote: > >> Hi Laszlo, > >> How could we configure the Qemu and test the UDF driver on OVMF? > > > > I guess you would format e.g. a DVD image with UDF, and attach it to > > QEMU like any other CD-ROM. > > I tried to look into this -- I tried several things, but nothing > produced an UDF image file that, when attached to the VM, would show up > in the UEFI shell as FSn: > > Google returned a bunch of pages, but all I found was: > - tips that didn't work (see above), > - confused users (like me) looking for solutions. > > So, at the moment, I have no idea how authoring UDF DVD images is > possible on Linux, so that they'd be recognized in edk2. > > (I'm interested in the edk2 UDF driver not because I want to "author" > UDF DVD images (ISO9660+ElTorito works just fine), but because some > optical media images that were given to me are formatted UDF-only. They > can be translated into ISO9660+ElTorito off-line, but that's a chore.) > > Thanks, > Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 10/09/2017 11:27, Shi, Steven wrote: > OK. Does the UDF image you created correctly show up as CD-ROM content in Linux, e.g Fedora? The Fedora image I have (Fedora-Workstation-Live-x86_64-26-1.5.iso) does not contain a valid UDF file system, so you cannot use it for testing. To make sure it really doesn't, I used a "Philips UDF Conformance Tool" that you can grab in [1], and the tool didn't find any valid UDF file system in it. I've been using an unmodified Windows 10 Enterprise image that does contain a valid UDF bridge disk image (ISO9660+ElTorito+UDF) to perform my tests. Additionally, I use an USB stick that I format it with 'sudo mkudffs -b 512 --media-type=hd /dev/sdX' and copy some files to it for testing BTW, when testing with OVMF AARCH64, I was using my USB stick that showed up correctly as 'fsX' in UEFI shell, but with the Windows 10 Enterprise ISO image, it didn't. I looked at the logs to see what was going on and I found out that the emulated CD-ROM drive is reporting a block size of 512 instead of 2048 -- which seems wrong to me. With 'qemu-system-x86_64 -cdrom', it reports a block size of 2048. The Partition driver is still able to find a valid ElTorito partition because, regardless the device block size, it always use a logical block size of 2048 starting at 32K. In UDF, when searching for AVDPs (Anchor Volume Descriptor Pointers), we search for them at fixed locations: 256, N - 256, N and 512 -- but, with a block size of 512, the locations change to 1024, N - 1024, N and 2048 -- thus breaking the volume recognition sequence. For testing the Windows image with a block size of 512, I used the comformance tool with './udf_test -verbose 60 -blocksize 512 ~/img/win_ent10.iso' and it failed to find a valid UDF file system. But with a block size of 2048, it worked. Is there any reason for reporting a block size of 512 when using '-cdrom' option in qemu-system-aarch64? Is that a bug? Or am I missing something here? Thanks! Paulo [1] - https://www.lscdweb.com/registered/udf_verifier.html > > > Steven Shi > Intel\SSG\STO\UEFI Firmware > > Tel: +86 021-61166522 > iNet: 821-6522 > >> -----Original Message----- >> From: Laszlo Ersek [mailto:lersek@redhat.com] >> Sent: Sunday, September 10, 2017 9:52 PM >> To: Shi, Steven <steven.shi@intel.com>; edk2-devel-01 <edk2- >> devel@lists.01.org> >> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Dong, Eric <eric.dong@intel.com>; Zeng, >> Star <star.zeng@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org> >> Subject: Re: [edk2] [PATCH 0/5] MdeModulePkg: UDF fixes and cleanups >> >> Hi Steven, >> >> On 09/10/17 10:38, Laszlo Ersek wrote: >>> On 09/10/17 06:24, Shi, Steven wrote: >>>> Hi Laszlo, >>>> How could we configure the Qemu and test the UDF driver on OVMF? >>> >>> I guess you would format e.g. a DVD image with UDF, and attach it to >>> QEMU like any other CD-ROM. >> >> I tried to look into this -- I tried several things, but nothing >> produced an UDF image file that, when attached to the VM, would show up >> in the UEFI shell as FSn: >> >> Google returned a bunch of pages, but all I found was: >> - tips that didn't work (see above), >> - confused users (like me) looking for solutions. >> >> So, at the moment, I have no idea how authoring UDF DVD images is >> possible on Linux, so that they'd be recognized in edk2. >> >> (I'm interested in the edk2 UDF driver not because I want to "author" >> UDF DVD images (ISO9660+ElTorito works just fine), but because some >> optical media images that were given to me are formatted UDF-only. They >> can be translated into ISO9660+ElTorito off-line, but that's a chore.) >> >> Thanks, >> Laszlo > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 09/10/17 17:51, Paulo Alcantara wrote: > > > On 10/09/2017 11:27, Shi, Steven wrote: >> OK. Does the UDF image you created correctly show up as CD-ROM content >> in Linux, e.g Fedora? > > The Fedora image I have (Fedora-Workstation-Live-x86_64-26-1.5.iso) does > not contain a valid UDF file system, so you cannot use it for testing. > > To make sure it really doesn't, I used a "Philips UDF Conformance Tool" > that you can grab in [1], and the tool didn't find any valid UDF file > system in it. > > I've been using an unmodified Windows 10 Enterprise image that does > contain a valid UDF bridge disk image (ISO9660+ElTorito+UDF) to perform > my tests. > > Additionally, I use an USB stick that I format it with 'sudo mkudffs -b > 512 --media-type=hd /dev/sdX' and copy some files to it for testing > > BTW, when testing with OVMF AARCH64, I was using my USB stick that > showed up correctly as 'fsX' in UEFI shell, but with the Windows 10 > Enterprise ISO image, it didn't. I looked at the logs to see what was > going on and I found out that the emulated CD-ROM drive is reporting a > block size of 512 instead of 2048 -- which seems wrong to me. With > 'qemu-system-x86_64 -cdrom', it reports a block size of 2048. > > The Partition driver is still able to find a valid ElTorito partition > because, regardless the device block size, it always use a logical block > size of 2048 starting at 32K. In UDF, when searching for AVDPs (Anchor > Volume Descriptor Pointers), we search for them at fixed locations: 256, > N - 256, N and 512 -- but, with a block size of 512, the locations > change to 1024, N - 1024, N and 2048 -- thus breaking the volume > recognition sequence. > > For testing the Windows image with a block size of 512, I used the > comformance tool with './udf_test -verbose 60 -blocksize 512 > ~/img/win_ent10.iso' and it failed to find a valid UDF file system. But > with a block size of 2048, it worked. > > Is there any reason for reporting a block size of 512 when using > '-cdrom' option in qemu-system-aarch64? Is that a bug? Or am I missing > something here? "-cdrom" is a legacy QEMU option, a shorthand that expands to a -drive option, and at least one -device option (it could expand to multiple -device options). And, this expansion can be different on different machine types. On i440fx machine types, -cdrom can mean an IDE CD, on q35, an AHCI CD, an aarch64/virt, a virtio-scsi-pci controller and a scsi-cd. For this reason, it is always best to use complete -drive and -device specifications (which is equivalent to saying it is best to always use libvirt). Now, the above does not imlpy that no bug can exist in this space. Can you run the "info qtree" monitor command, in both cases, and compare the output with regard to the CD-ROM? Thanks Laszlo > Thanks! > Paulo > > [1] - https://www.lscdweb.com/registered/udf_verifier.html _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 11/09/2017 03:58, Laszlo Ersek wrote: > On 09/10/17 17:51, Paulo Alcantara wrote: >> >> >> On 10/09/2017 11:27, Shi, Steven wrote: >>> OK. Does the UDF image you created correctly show up as CD-ROM content >>> in Linux, e.g Fedora? >> >> The Fedora image I have (Fedora-Workstation-Live-x86_64-26-1.5.iso) does >> not contain a valid UDF file system, so you cannot use it for testing. >> >> To make sure it really doesn't, I used a "Philips UDF Conformance Tool" >> that you can grab in [1], and the tool didn't find any valid UDF file >> system in it. >> >> I've been using an unmodified Windows 10 Enterprise image that does >> contain a valid UDF bridge disk image (ISO9660+ElTorito+UDF) to perform >> my tests. >> >> Additionally, I use an USB stick that I format it with 'sudo mkudffs -b >> 512 --media-type=hd /dev/sdX' and copy some files to it for testing >> >> BTW, when testing with OVMF AARCH64, I was using my USB stick that >> showed up correctly as 'fsX' in UEFI shell, but with the Windows 10 >> Enterprise ISO image, it didn't. I looked at the logs to see what was >> going on and I found out that the emulated CD-ROM drive is reporting a >> block size of 512 instead of 2048 -- which seems wrong to me. With >> 'qemu-system-x86_64 -cdrom', it reports a block size of 2048. >> >> The Partition driver is still able to find a valid ElTorito partition >> because, regardless the device block size, it always use a logical block >> size of 2048 starting at 32K. In UDF, when searching for AVDPs (Anchor >> Volume Descriptor Pointers), we search for them at fixed locations: 256, >> N - 256, N and 512 -- but, with a block size of 512, the locations >> change to 1024, N - 1024, N and 2048 -- thus breaking the volume >> recognition sequence. >> >> For testing the Windows image with a block size of 512, I used the >> comformance tool with './udf_test -verbose 60 -blocksize 512 >> ~/img/win_ent10.iso' and it failed to find a valid UDF file system. But >> with a block size of 2048, it worked. >> >> Is there any reason for reporting a block size of 512 when using >> '-cdrom' option in qemu-system-aarch64? Is that a bug? Or am I missing >> something here? > > "-cdrom" is a legacy QEMU option, a shorthand that expands to a -drive > option, and at least one -device option (it could expand to multiple > -device options). And, this expansion can be different on different > machine types. On i440fx machine types, -cdrom can mean an IDE CD, on > q35, an AHCI CD, an aarch64/virt, a virtio-scsi-pci controller and a > scsi-cd. For this reason, it is always best to use complete -drive and > -device specifications (which is equivalent to saying it is best to > always use libvirt). > > Now, the above does not imlpy that no bug can exist in this space. Can > you run the "info qtree" monitor command, in both cases, and compare the > output with regard to the CD-ROM? OK. Thanks for clarifying that to me. I'll do it when I have a chance and tell you. Paulo > > Thanks > Laszlo > >> Thanks! >> Paulo >> >> [1] - https://www.lscdweb.com/registered/udf_verifier.html _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Hi Laszlo,
Your code is good. But there is a "Null Pointer Use" issue, which is a undefined behavior in C spec, in edk2/MdeModulePkg/Universal/Disk/UdfDxe/File.c, line:696, column: 7 and line:707, column: 14.
Line695:
FileIdentifierDesc = PrivFileData->File.FileIdentifierDesc;
if (IS_FID_DIRECTORY_FILE (FileIdentifierDesc)) {
...
The above FileIdentifierDesc can be NULL if you run a .efi in a Udf partition. And the later IS_FID_DIRECTORY_FILE (FileIdentifierDesc) is unsafe if FileIdentifierDesc == NULL. You can test it with below extra code which add debug output when FileIdentifierDesc == NULL, and check the log after load and run something in a Udf partition.
+++ b/MdeModulePkg/Universal/Disk/UdfDxe/File.c
@@ -693,6 +693,11 @@ UdfSetPosition (
PrivFileData = PRIVATE_UDF_FILE_DATA_FROM_THIS (This);
FileIdentifierDesc = PrivFileData->File.FileIdentifierDesc;
+ if (FileIdentifierDesc == NULL) {
+ DEBUG ((EFI_D_ERROR, "FileIdentifierDesc == NULL!\n"));
+ return Status;
+ }
+
if (IS_FID_DIRECTORY_FILE (FileIdentifierDesc)) {
//
// If the file handle is a directory, the _only_ position that may be set is
I followed the Paulo's suggestion, and found this issue when using the Udf partition of Windows 10 Enterprise ISO image on qemu. Below is my qemu command:
/usr/local/bin/qemu-system-x86_64 -m 5120 -enable-kvm -machine pc-q35-2.9 -bios OVMF.fd -serial file:serial.log -cdrom /media/jshi19/My\ Passport/OSImage/ISO/winserver/en_windows_server_2016_x64_dvd_9327751.iso
I've submitted a bug for this issue: https://bugzilla.tianocore.org/show_bug.cgi?id=704.
I found this issue by edk2 llvm undefined behavior sanitizer, and below is the sanitizer output. FYI.
/home/jshi19/wksp_efi/edk2/MdeModulePkg/Universal/Disk/UdfDxe/File.c, line:0x02B8, column:0x0007 ErrorType = NullPointerUse: member access within null pointer of type 'UDF_FILE_IDENTIFIER_DESCRIPTOR'
ASAN MEMORY ACCESS check fail! __ubsan_handle_type_mismatch_v1 is called:
UdfSetPosition
/home/jshi19/wksp_efi/edk2/MdeModulePkg/Universal/Disk/UdfDxe/File.c:696:7
EfiShellFindFilesInDir
/home/jshi19/wksp_efi/edk2/ShellPkg/Application/Shell/ShellProtocol.c:2136:18
ShellSearchHandle
/home/jshi19/wksp_efi/edk2/ShellPkg/Application/Shell/ShellProtocol.c:2413:14
EfiShellFindFiles
/home/jshi19/wksp_efi/edk2/ShellPkg/Application/Shell/ShellProtocol.c:2586:18
EfiShellOpenFileList
/home/jshi19/wksp_efi/edk2/ShellPkg/Application/Shell/ShellProtocol.c:2666:12
ShellOpenFileMetaArg
/home/jshi19/wksp_efi/edk2/ShellPkg/Library/UefiShellLib/UefiShellLib.c:1570:14
/home/jshi19/wksp_efi/edk2/MdeModulePkg/Universal/Disk/UdfDxe/File.c, line:0x02C3, column:0x000E ErrorType = NullPointerUse: member access within null pointer of type 'UDF_FILE_IDENTIFIER_DESCRIPTOR'
ASAN MEMORY ACCESS check fail! __ubsan_handle_type_mismatch_v1 is called:
...
Steven Shi
Intel\SSG\STO\UEFI Firmware
Tel: +86 021-61166522
iNet: 821-6522
> -----Original Message-----
> From: Paulo Alcantara [mailto:pcacjr@zytor.com]
> Sent: Sunday, September 10, 2017 11:52 PM
> To: Shi, Steven <steven.shi@intel.com>; Laszlo Ersek <lersek@redhat.com>;
> edk2-devel-01 <edk2-devel@lists.01.org>
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Dong, Eric <eric.dong@intel.com>; Zeng,
> Star <star.zeng@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Subject: Re: [edk2] [PATCH 0/5] MdeModulePkg: UDF fixes and cleanups
>
>
>
> On 10/09/2017 11:27, Shi, Steven wrote:
> > OK. Does the UDF image you created correctly show up as CD-ROM
> content in Linux, e.g Fedora?
>
> The Fedora image I have (Fedora-Workstation-Live-x86_64-26-1.5.iso) does
> not contain a valid UDF file system, so you cannot use it for testing.
>
> To make sure it really doesn't, I used a "Philips UDF Conformance Tool"
> that you can grab in [1], and the tool didn't find any valid UDF file
> system in it.
>
> I've been using an unmodified Windows 10 Enterprise image that does
> contain a valid UDF bridge disk image (ISO9660+ElTorito+UDF) to perform
> my tests.
>
> Additionally, I use an USB stick that I format it with 'sudo mkudffs -b
> 512 --media-type=hd /dev/sdX' and copy some files to it for testing
>
> BTW, when testing with OVMF AARCH64, I was using my USB stick that
> showed up correctly as 'fsX' in UEFI shell, but with the Windows 10
> Enterprise ISO image, it didn't. I looked at the logs to see what was
> going on and I found out that the emulated CD-ROM drive is reporting a
> block size of 512 instead of 2048 -- which seems wrong to me. With
> 'qemu-system-x86_64 -cdrom', it reports a block size of 2048.
>
> The Partition driver is still able to find a valid ElTorito partition
> because, regardless the device block size, it always use a logical block
> size of 2048 starting at 32K. In UDF, when searching for AVDPs (Anchor
> Volume Descriptor Pointers), we search for them at fixed locations: 256,
> N - 256, N and 512 -- but, with a block size of 512, the locations
> change to 1024, N - 1024, N and 2048 -- thus breaking the volume
> recognition sequence.
>
> For testing the Windows image with a block size of 512, I used the
> comformance tool with './udf_test -verbose 60 -blocksize 512
> ~/img/win_ent10.iso' and it failed to find a valid UDF file system. But
> with a block size of 2048, it worked.
>
> Is there any reason for reporting a block size of 512 when using
> '-cdrom' option in qemu-system-aarch64? Is that a bug? Or am I missing
> something here?
>
> Thanks!
> Paulo
>
> [1] - https://www.lscdweb.com/registered/udf_verifier.html
>
> >
> >
> > Steven Shi
> > Intel\SSG\STO\UEFI Firmware
> >
> > Tel: +86 021-61166522
> > iNet: 821-6522
> >
> >> -----Original Message-----
> >> From: Laszlo Ersek [mailto:lersek@redhat.com]
> >> Sent: Sunday, September 10, 2017 9:52 PM
> >> To: Shi, Steven <steven.shi@intel.com>; edk2-devel-01 <edk2-
> >> devel@lists.01.org>
> >> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Dong, Eric <eric.dong@intel.com>;
> Zeng,
> >> Star <star.zeng@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >> Subject: Re: [edk2] [PATCH 0/5] MdeModulePkg: UDF fixes and cleanups
> >>
> >> Hi Steven,
> >>
> >> On 09/10/17 10:38, Laszlo Ersek wrote:
> >>> On 09/10/17 06:24, Shi, Steven wrote:
> >>>> Hi Laszlo,
> >>>> How could we configure the Qemu and test the UDF driver on OVMF?
> >>>
> >>> I guess you would format e.g. a DVD image with UDF, and attach it to
> >>> QEMU like any other CD-ROM.
> >>
> >> I tried to look into this -- I tried several things, but nothing
> >> produced an UDF image file that, when attached to the VM, would show
> up
> >> in the UEFI shell as FSn:
> >>
> >> Google returned a bunch of pages, but all I found was:
> >> - tips that didn't work (see above),
> >> - confused users (like me) looking for solutions.
> >>
> >> So, at the moment, I have no idea how authoring UDF DVD images is
> >> possible on Linux, so that they'd be recognized in edk2.
> >>
> >> (I'm interested in the edk2 UDF driver not because I want to "author"
> >> UDF DVD images (ISO9660+ElTorito works just fine), but because some
> >> optical media images that were given to me are formatted UDF-only.
> They
> >> can be translated into ISO9660+ElTorito off-line, but that's a chore.)
> >>
> >> Thanks,
> >> Laszlo
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel
> >
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Steven,
Thanks for capturing the bug using ASAN!
-----Original Message-----
From: Shi, Steven
Sent: Monday, September 11, 2017 9:08 PM
To: Paulo Alcantara <pcacjr@zytor.com>; Laszlo Ersek <lersek@redhat.com>; edk2-devel-01 <edk2-devel@lists.01.org>
Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Dong, Eric <eric.dong@intel.com>; Zeng, Star <star.zeng@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
Subject: RE: [edk2] [PATCH 0/5] MdeModulePkg: UDF fixes and cleanups
Hi Laszlo,
Your code is good. But there is a "Null Pointer Use" issue, which is a undefined behavior in C spec, in edk2/MdeModulePkg/Universal/Disk/UdfDxe/File.c, line:696, column: 7 and line:707, column: 14.
Line695:
FileIdentifierDesc = PrivFileData->File.FileIdentifierDesc;
if (IS_FID_DIRECTORY_FILE (FileIdentifierDesc)) {
...
The above FileIdentifierDesc can be NULL if you run a .efi in a Udf partition. And the later IS_FID_DIRECTORY_FILE (FileIdentifierDesc) is unsafe if FileIdentifierDesc == NULL. You can test it with below extra code which add debug output when FileIdentifierDesc == NULL, and check the log after load and run something in a Udf partition.
+++ b/MdeModulePkg/Universal/Disk/UdfDxe/File.c
@@ -693,6 +693,11 @@ UdfSetPosition (
PrivFileData = PRIVATE_UDF_FILE_DATA_FROM_THIS (This);
FileIdentifierDesc = PrivFileData->File.FileIdentifierDesc;
+ if (FileIdentifierDesc == NULL) {
+ DEBUG ((EFI_D_ERROR, "FileIdentifierDesc == NULL!\n"));
+ return Status;
+ }
+
if (IS_FID_DIRECTORY_FILE (FileIdentifierDesc)) {
//
// If the file handle is a directory, the _only_ position that may be set is
I followed the Paulo's suggestion, and found this issue when using the Udf partition of Windows 10 Enterprise ISO image on qemu. Below is my qemu command:
/usr/local/bin/qemu-system-x86_64 -m 5120 -enable-kvm -machine pc-q35-2.9 -bios OVMF.fd -serial file:serial.log -cdrom /media/jshi19/My\ Passport/OSImage/ISO/winserver/en_windows_server_2016_x64_dvd_9327751.iso
I've submitted a bug for this issue: https://bugzilla.tianocore.org/show_bug.cgi?id=704.
I found this issue by edk2 llvm undefined behavior sanitizer, and below is the sanitizer output. FYI.
/home/jshi19/wksp_efi/edk2/MdeModulePkg/Universal/Disk/UdfDxe/File.c, line:0x02B8, column:0x0007 ErrorType = NullPointerUse: member access within null pointer of type 'UDF_FILE_IDENTIFIER_DESCRIPTOR'
ASAN MEMORY ACCESS check fail! __ubsan_handle_type_mismatch_v1 is called:
UdfSetPosition
/home/jshi19/wksp_efi/edk2/MdeModulePkg/Universal/Disk/UdfDxe/File.c:696:7
EfiShellFindFilesInDir
/home/jshi19/wksp_efi/edk2/ShellPkg/Application/Shell/ShellProtocol.c:2136:18
ShellSearchHandle
/home/jshi19/wksp_efi/edk2/ShellPkg/Application/Shell/ShellProtocol.c:2413:14
EfiShellFindFiles
/home/jshi19/wksp_efi/edk2/ShellPkg/Application/Shell/ShellProtocol.c:2586:18
EfiShellOpenFileList
/home/jshi19/wksp_efi/edk2/ShellPkg/Application/Shell/ShellProtocol.c:2666:12
ShellOpenFileMetaArg
/home/jshi19/wksp_efi/edk2/ShellPkg/Library/UefiShellLib/UefiShellLib.c:1570:14
/home/jshi19/wksp_efi/edk2/MdeModulePkg/Universal/Disk/UdfDxe/File.c, line:0x02C3, column:0x000E ErrorType = NullPointerUse: member access within null pointer of type 'UDF_FILE_IDENTIFIER_DESCRIPTOR'
ASAN MEMORY ACCESS check fail! __ubsan_handle_type_mismatch_v1 is called:
...
Steven Shi
Intel\SSG\STO\UEFI Firmware
Tel: +86 021-61166522
iNet: 821-6522
> -----Original Message-----
> From: Paulo Alcantara [mailto:pcacjr@zytor.com]
> Sent: Sunday, September 10, 2017 11:52 PM
> To: Shi, Steven <steven.shi@intel.com>; Laszlo Ersek
> <lersek@redhat.com>;
> edk2-devel-01 <edk2-devel@lists.01.org>
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Dong, Eric <eric.dong@intel.com>;
> Zeng, Star <star.zeng@intel.com>; Ard Biesheuvel
> <ard.biesheuvel@linaro.org>
> Subject: Re: [edk2] [PATCH 0/5] MdeModulePkg: UDF fixes and cleanups
>
>
>
> On 10/09/2017 11:27, Shi, Steven wrote:
> > OK. Does the UDF image you created correctly show up as CD-ROM
> content in Linux, e.g Fedora?
>
> The Fedora image I have (Fedora-Workstation-Live-x86_64-26-1.5.iso)
> does not contain a valid UDF file system, so you cannot use it for testing.
>
> To make sure it really doesn't, I used a "Philips UDF Conformance Tool"
> that you can grab in [1], and the tool didn't find any valid UDF file
> system in it.
>
> I've been using an unmodified Windows 10 Enterprise image that does
> contain a valid UDF bridge disk image (ISO9660+ElTorito+UDF) to
> perform my tests.
>
> Additionally, I use an USB stick that I format it with 'sudo mkudffs
> -b
> 512 --media-type=hd /dev/sdX' and copy some files to it for testing
>
> BTW, when testing with OVMF AARCH64, I was using my USB stick that
> showed up correctly as 'fsX' in UEFI shell, but with the Windows 10
> Enterprise ISO image, it didn't. I looked at the logs to see what was
> going on and I found out that the emulated CD-ROM drive is reporting a
> block size of 512 instead of 2048 -- which seems wrong to me. With
> 'qemu-system-x86_64 -cdrom', it reports a block size of 2048.
>
> The Partition driver is still able to find a valid ElTorito partition
> because, regardless the device block size, it always use a logical
> block size of 2048 starting at 32K. In UDF, when searching for AVDPs
> (Anchor Volume Descriptor Pointers), we search for them at fixed
> locations: 256, N - 256, N and 512 -- but, with a block size of 512,
> the locations change to 1024, N - 1024, N and 2048 -- thus breaking
> the volume recognition sequence.
>
> For testing the Windows image with a block size of 512, I used the
> comformance tool with './udf_test -verbose 60 -blocksize 512
> ~/img/win_ent10.iso' and it failed to find a valid UDF file system.
> But with a block size of 2048, it worked.
>
> Is there any reason for reporting a block size of 512 when using
> '-cdrom' option in qemu-system-aarch64? Is that a bug? Or am I missing
> something here?
>
> Thanks!
> Paulo
>
> [1] - https://www.lscdweb.com/registered/udf_verifier.html
>
> >
> >
> > Steven Shi
> > Intel\SSG\STO\UEFI Firmware
> >
> > Tel: +86 021-61166522
> > iNet: 821-6522
> >
> >> -----Original Message-----
> >> From: Laszlo Ersek [mailto:lersek@redhat.com]
> >> Sent: Sunday, September 10, 2017 9:52 PM
> >> To: Shi, Steven <steven.shi@intel.com>; edk2-devel-01 <edk2-
> >> devel@lists.01.org>
> >> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Dong, Eric
> >> <eric.dong@intel.com>;
> Zeng,
> >> Star <star.zeng@intel.com>; Ard Biesheuvel
> >> <ard.biesheuvel@linaro.org>
> >> Subject: Re: [edk2] [PATCH 0/5] MdeModulePkg: UDF fixes and
> >> cleanups
> >>
> >> Hi Steven,
> >>
> >> On 09/10/17 10:38, Laszlo Ersek wrote:
> >>> On 09/10/17 06:24, Shi, Steven wrote:
> >>>> Hi Laszlo,
> >>>> How could we configure the Qemu and test the UDF driver on OVMF?
> >>>
> >>> I guess you would format e.g. a DVD image with UDF, and attach it
> >>> to QEMU like any other CD-ROM.
> >>
> >> I tried to look into this -- I tried several things, but nothing
> >> produced an UDF image file that, when attached to the VM, would
> >> show
> up
> >> in the UEFI shell as FSn:
> >>
> >> Google returned a bunch of pages, but all I found was:
> >> - tips that didn't work (see above),
> >> - confused users (like me) looking for solutions.
> >>
> >> So, at the moment, I have no idea how authoring UDF DVD images is
> >> possible on Linux, so that they'd be recognized in edk2.
> >>
> >> (I'm interested in the edk2 UDF driver not because I want to "author"
> >> UDF DVD images (ISO9660+ElTorito works just fine), but because some
> >> optical media images that were given to me are formatted UDF-only.
> They
> >> can be translated into ISO9660+ElTorito off-line, but that's a
> >> chore.)
> >>
> >> Thanks,
> >> Laszlo
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel
> >
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
On 09/11/17 15:07, Shi, Steven wrote:
> Hi Laszlo,
> Your code is good. But there is a "Null Pointer Use" issue, which is a undefined behavior in C spec, in edk2/MdeModulePkg/Universal/Disk/UdfDxe/File.c, line:696, column: 7 and line:707, column: 14.
>
> Line695:
> FileIdentifierDesc = PrivFileData->File.FileIdentifierDesc;
> if (IS_FID_DIRECTORY_FILE (FileIdentifierDesc)) {
> ...
>
> The above FileIdentifierDesc can be NULL if you run a .efi in a Udf partition. And the later IS_FID_DIRECTORY_FILE (FileIdentifierDesc) is unsafe if FileIdentifierDesc == NULL. You can test it with below extra code which add debug output when FileIdentifierDesc == NULL, and check the log after load and run something in a Udf partition.
>
> +++ b/MdeModulePkg/Universal/Disk/UdfDxe/File.c
> @@ -693,6 +693,11 @@ UdfSetPosition (
> PrivFileData = PRIVATE_UDF_FILE_DATA_FROM_THIS (This);
>
> FileIdentifierDesc = PrivFileData->File.FileIdentifierDesc;
> + if (FileIdentifierDesc == NULL) {
> + DEBUG ((EFI_D_ERROR, "FileIdentifierDesc == NULL!\n"));
> + return Status;
> + }
> +
> if (IS_FID_DIRECTORY_FILE (FileIdentifierDesc)) {
> //
> // If the file handle is a directory, the _only_ position that may be set is
>
> I followed the Paulo's suggestion, and found this issue when using the Udf partition of Windows 10 Enterprise ISO image on qemu. Below is my qemu command:
> /usr/local/bin/qemu-system-x86_64 -m 5120 -enable-kvm -machine pc-q35-2.9 -bios OVMF.fd -serial file:serial.log -cdrom /media/jshi19/My\ Passport/OSImage/ISO/winserver/en_windows_server_2016_x64_dvd_9327751.iso
>
> I've submitted a bug for this issue: https://bugzilla.tianocore.org/show_bug.cgi?id=704.
Right, I saw the report (via
<https://lists.01.org/mailman/listinfo/edk2-bugs>), thank you for filing it.
I asked Paulo a few minutes ago to register in the TianoCore Bugzilla,
and to take the BZ.
I don't intend to co-maintain UdfDxe permanently, aside from one-off
patches and reviews, exactly like with any other MdeModulePkg driver.
I'm totally unfamiliar with UdfDxe code, I'm just interested in using
the feature. The only reason I sent these five patches over the weekend
is that the initial code drop, technically committed by me, broke the
build with CLANG38, and Paulo wasn't near his computer. I wanted to fix
the build breakage ASAP.
If you find other issues, please continue filing BZs, and Paulo should
please continue helping out with them.
Thanks!
Laszlo
>
> I found this issue by edk2 llvm undefined behavior sanitizer, and below is the sanitizer output. FYI.
> /home/jshi19/wksp_efi/edk2/MdeModulePkg/Universal/Disk/UdfDxe/File.c, line:0x02B8, column:0x0007 ErrorType = NullPointerUse: member access within null pointer of type 'UDF_FILE_IDENTIFIER_DESCRIPTOR'
> ASAN MEMORY ACCESS check fail! __ubsan_handle_type_mismatch_v1 is called:
> UdfSetPosition
> /home/jshi19/wksp_efi/edk2/MdeModulePkg/Universal/Disk/UdfDxe/File.c:696:7
> EfiShellFindFilesInDir
> /home/jshi19/wksp_efi/edk2/ShellPkg/Application/Shell/ShellProtocol.c:2136:18
> ShellSearchHandle
> /home/jshi19/wksp_efi/edk2/ShellPkg/Application/Shell/ShellProtocol.c:2413:14
> EfiShellFindFiles
> /home/jshi19/wksp_efi/edk2/ShellPkg/Application/Shell/ShellProtocol.c:2586:18
> EfiShellOpenFileList
> /home/jshi19/wksp_efi/edk2/ShellPkg/Application/Shell/ShellProtocol.c:2666:12
> ShellOpenFileMetaArg
> /home/jshi19/wksp_efi/edk2/ShellPkg/Library/UefiShellLib/UefiShellLib.c:1570:14
>
> /home/jshi19/wksp_efi/edk2/MdeModulePkg/Universal/Disk/UdfDxe/File.c, line:0x02C3, column:0x000E ErrorType = NullPointerUse: member access within null pointer of type 'UDF_FILE_IDENTIFIER_DESCRIPTOR'
> ASAN MEMORY ACCESS check fail! __ubsan_handle_type_mismatch_v1 is called:
> ...
>
> Steven Shi
> Intel\SSG\STO\UEFI Firmware
>
> Tel: +86 021-61166522
> iNet: 821-6522
>
>> -----Original Message-----
>> From: Paulo Alcantara [mailto:pcacjr@zytor.com]
>> Sent: Sunday, September 10, 2017 11:52 PM
>> To: Shi, Steven <steven.shi@intel.com>; Laszlo Ersek <lersek@redhat.com>;
>> edk2-devel-01 <edk2-devel@lists.01.org>
>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Dong, Eric <eric.dong@intel.com>; Zeng,
>> Star <star.zeng@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Subject: Re: [edk2] [PATCH 0/5] MdeModulePkg: UDF fixes and cleanups
>>
>>
>>
>> On 10/09/2017 11:27, Shi, Steven wrote:
>>> OK. Does the UDF image you created correctly show up as CD-ROM
>> content in Linux, e.g Fedora?
>>
>> The Fedora image I have (Fedora-Workstation-Live-x86_64-26-1.5.iso) does
>> not contain a valid UDF file system, so you cannot use it for testing.
>>
>> To make sure it really doesn't, I used a "Philips UDF Conformance Tool"
>> that you can grab in [1], and the tool didn't find any valid UDF file
>> system in it.
>>
>> I've been using an unmodified Windows 10 Enterprise image that does
>> contain a valid UDF bridge disk image (ISO9660+ElTorito+UDF) to perform
>> my tests.
>>
>> Additionally, I use an USB stick that I format it with 'sudo mkudffs -b
>> 512 --media-type=hd /dev/sdX' and copy some files to it for testing
>>
>> BTW, when testing with OVMF AARCH64, I was using my USB stick that
>> showed up correctly as 'fsX' in UEFI shell, but with the Windows 10
>> Enterprise ISO image, it didn't. I looked at the logs to see what was
>> going on and I found out that the emulated CD-ROM drive is reporting a
>> block size of 512 instead of 2048 -- which seems wrong to me. With
>> 'qemu-system-x86_64 -cdrom', it reports a block size of 2048.
>>
>> The Partition driver is still able to find a valid ElTorito partition
>> because, regardless the device block size, it always use a logical block
>> size of 2048 starting at 32K. In UDF, when searching for AVDPs (Anchor
>> Volume Descriptor Pointers), we search for them at fixed locations: 256,
>> N - 256, N and 512 -- but, with a block size of 512, the locations
>> change to 1024, N - 1024, N and 2048 -- thus breaking the volume
>> recognition sequence.
>>
>> For testing the Windows image with a block size of 512, I used the
>> comformance tool with './udf_test -verbose 60 -blocksize 512
>> ~/img/win_ent10.iso' and it failed to find a valid UDF file system. But
>> with a block size of 2048, it worked.
>>
>> Is there any reason for reporting a block size of 512 when using
>> '-cdrom' option in qemu-system-aarch64? Is that a bug? Or am I missing
>> something here?
>>
>> Thanks!
>> Paulo
>>
>> [1] - https://www.lscdweb.com/registered/udf_verifier.html
>>
>>>
>>>
>>> Steven Shi
>>> Intel\SSG\STO\UEFI Firmware
>>>
>>> Tel: +86 021-61166522
>>> iNet: 821-6522
>>>
>>>> -----Original Message-----
>>>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>>>> Sent: Sunday, September 10, 2017 9:52 PM
>>>> To: Shi, Steven <steven.shi@intel.com>; edk2-devel-01 <edk2-
>>>> devel@lists.01.org>
>>>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Dong, Eric <eric.dong@intel.com>;
>> Zeng,
>>>> Star <star.zeng@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>>> Subject: Re: [edk2] [PATCH 0/5] MdeModulePkg: UDF fixes and cleanups
>>>>
>>>> Hi Steven,
>>>>
>>>> On 09/10/17 10:38, Laszlo Ersek wrote:
>>>>> On 09/10/17 06:24, Shi, Steven wrote:
>>>>>> Hi Laszlo,
>>>>>> How could we configure the Qemu and test the UDF driver on OVMF?
>>>>>
>>>>> I guess you would format e.g. a DVD image with UDF, and attach it to
>>>>> QEMU like any other CD-ROM.
>>>>
>>>> I tried to look into this -- I tried several things, but nothing
>>>> produced an UDF image file that, when attached to the VM, would show
>> up
>>>> in the UEFI shell as FSn:
>>>>
>>>> Google returned a bunch of pages, but all I found was:
>>>> - tips that didn't work (see above),
>>>> - confused users (like me) looking for solutions.
>>>>
>>>> So, at the moment, I have no idea how authoring UDF DVD images is
>>>> possible on Linux, so that they'd be recognized in edk2.
>>>>
>>>> (I'm interested in the edk2 UDF driver not because I want to "author"
>>>> UDF DVD images (ISO9660+ElTorito works just fine), but because some
>>>> optical media images that were given to me are formatted UDF-only.
>> They
>>>> can be translated into ISO9660+ElTorito off-line, but that's a chore.)
>>>>
>>>> Thanks,
>>>> Laszlo
>>> _______________________________________________
>>> edk2-devel mailing list
>>> edk2-devel@lists.01.org
>>> https://lists.01.org/mailman/listinfo/edk2-devel
>>>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Hi Steven,
On 11/09/2017 10:07, Shi, Steven wrote:
> Hi Laszlo,
> Your code is good. But there is a "Null Pointer Use" issue, which is a undefined behavior in C spec, in edk2/MdeModulePkg/Universal/Disk/UdfDxe/File.c, line:696, column: 7 and line:707, column: 14.
>
> Line695:
> FileIdentifierDesc = PrivFileData->File.FileIdentifierDesc;
> if (IS_FID_DIRECTORY_FILE (FileIdentifierDesc)) {
> ...
>
> The above FileIdentifierDesc can be NULL if you run a .efi in a Udf partition. And the later IS_FID_DIRECTORY_FILE (FileIdentifierDesc) is unsafe if FileIdentifierDesc == NULL. You can test it with below extra code which add debug output when FileIdentifierDesc == NULL, and check the log after load and run something in a Udf partition.
>
> +++ b/MdeModulePkg/Universal/Disk/UdfDxe/File.c
> @@ -693,6 +693,11 @@ UdfSetPosition (
> PrivFileData = PRIVATE_UDF_FILE_DATA_FROM_THIS (This);
>
> FileIdentifierDesc = PrivFileData->File.FileIdentifierDesc;
> + if (FileIdentifierDesc == NULL) {
> + DEBUG ((EFI_D_ERROR, "FileIdentifierDesc == NULL!\n"));
> + return Status;
> + }
> +
> if (IS_FID_DIRECTORY_FILE (FileIdentifierDesc)) {
> //
> // If the file handle is a directory, the _only_ position that may be set is
>
> I followed the Paulo's suggestion, and found this issue when using the Udf partition of Windows 10 Enterprise ISO image on qemu. Below is my qemu command:
> /usr/local/bin/qemu-system-x86_64 -m 5120 -enable-kvm -machine pc-q35-2.9 -bios OVMF.fd -serial file:serial.log -cdrom /media/jshi19/My\ Passport/OSImage/ISO/winserver/en_windows_server_2016_x64_dvd_9327751.iso
>
> I've submitted a bug for this issue: https://bugzilla.tianocore.org/show_bug.cgi?id=704.
>
> I found this issue by edk2 llvm undefined behavior sanitizer, and below is the sanitizer output. FYI.
> /home/jshi19/wksp_efi/edk2/MdeModulePkg/Universal/Disk/UdfDxe/File.c, line:0x02B8, column:0x0007 ErrorType = NullPointerUse: member access within null pointer of type 'UDF_FILE_IDENTIFIER_DESCRIPTOR'
> ASAN MEMORY ACCESS check fail! __ubsan_handle_type_mismatch_v1 is called:
> UdfSetPosition
> /home/jshi19/wksp_efi/edk2/MdeModulePkg/Universal/Disk/UdfDxe/File.c:696:7
> EfiShellFindFilesInDir
> /home/jshi19/wksp_efi/edk2/ShellPkg/Application/Shell/ShellProtocol.c:2136:18
> ShellSearchHandle
> /home/jshi19/wksp_efi/edk2/ShellPkg/Application/Shell/ShellProtocol.c:2413:14
> EfiShellFindFiles
> /home/jshi19/wksp_efi/edk2/ShellPkg/Application/Shell/ShellProtocol.c:2586:18
> EfiShellOpenFileList
> /home/jshi19/wksp_efi/edk2/ShellPkg/Application/Shell/ShellProtocol.c:2666:12
> ShellOpenFileMetaArg
> /home/jshi19/wksp_efi/edk2/ShellPkg/Library/UefiShellLib/UefiShellLib.c:1570:14
>
> /home/jshi19/wksp_efi/edk2/MdeModulePkg/Universal/Disk/UdfDxe/File.c, line:0x02C3, column:0x000E ErrorType = NullPointerUse: member access within null pointer of type 'UDF_FILE_IDENTIFIER_DESCRIPTOR'
> ASAN MEMORY ACCESS check fail! __ubsan_handle_type_mismatch_v1 is called:
> ...
Thank you for catching this bug up. I think this issue is related to
accessing a File Identifier Descriptor from a root directory file --
e.g., this should be "FileIdentifierDesc =
PrivFileData->Root->FileIdentifierDesc;"
Could you please replace the broken assignment with:
FileIdentifierDesc = _FILE(PrivFileData)->FileIdentifierDesc;
ASSERT (FileIdentifierDesc != NULL);
...
Tell me if that worked for you. If so, I could send a formal patch
fixing this issue later.
(BTW, I do apologize the late replies but I'm only able to work on this
in my free time)
Thanks,
Paulo
>
> Steven Shi
> Intel\SSG\STO\UEFI Firmware
>
> Tel: +86 021-61166522
> iNet: 821-6522
>
>> -----Original Message-----
>> From: Paulo Alcantara [mailto:pcacjr@zytor.com]
>> Sent: Sunday, September 10, 2017 11:52 PM
>> To: Shi, Steven <steven.shi@intel.com>; Laszlo Ersek <lersek@redhat.com>;
>> edk2-devel-01 <edk2-devel@lists.01.org>
>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Dong, Eric <eric.dong@intel.com>; Zeng,
>> Star <star.zeng@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Subject: Re: [edk2] [PATCH 0/5] MdeModulePkg: UDF fixes and cleanups
>>
>>
>>
>> On 10/09/2017 11:27, Shi, Steven wrote:
>>> OK. Does the UDF image you created correctly show up as CD-ROM
>> content in Linux, e.g Fedora?
>>
>> The Fedora image I have (Fedora-Workstation-Live-x86_64-26-1.5.iso) does
>> not contain a valid UDF file system, so you cannot use it for testing.
>>
>> To make sure it really doesn't, I used a "Philips UDF Conformance Tool"
>> that you can grab in [1], and the tool didn't find any valid UDF file
>> system in it.
>>
>> I've been using an unmodified Windows 10 Enterprise image that does
>> contain a valid UDF bridge disk image (ISO9660+ElTorito+UDF) to perform
>> my tests.
>>
>> Additionally, I use an USB stick that I format it with 'sudo mkudffs -b
>> 512 --media-type=hd /dev/sdX' and copy some files to it for testing
>>
>> BTW, when testing with OVMF AARCH64, I was using my USB stick that
>> showed up correctly as 'fsX' in UEFI shell, but with the Windows 10
>> Enterprise ISO image, it didn't. I looked at the logs to see what was
>> going on and I found out that the emulated CD-ROM drive is reporting a
>> block size of 512 instead of 2048 -- which seems wrong to me. With
>> 'qemu-system-x86_64 -cdrom', it reports a block size of 2048.
>>
>> The Partition driver is still able to find a valid ElTorito partition
>> because, regardless the device block size, it always use a logical block
>> size of 2048 starting at 32K. In UDF, when searching for AVDPs (Anchor
>> Volume Descriptor Pointers), we search for them at fixed locations: 256,
>> N - 256, N and 512 -- but, with a block size of 512, the locations
>> change to 1024, N - 1024, N and 2048 -- thus breaking the volume
>> recognition sequence.
>>
>> For testing the Windows image with a block size of 512, I used the
>> comformance tool with './udf_test -verbose 60 -blocksize 512
>> ~/img/win_ent10.iso' and it failed to find a valid UDF file system. But
>> with a block size of 2048, it worked.
>>
>> Is there any reason for reporting a block size of 512 when using
>> '-cdrom' option in qemu-system-aarch64? Is that a bug? Or am I missing
>> something here?
>>
>> Thanks!
>> Paulo
>>
>> [1] - https://www.lscdweb.com/registered/udf_verifier.html
>>
>>>
>>>
>>> Steven Shi
>>> Intel\SSG\STO\UEFI Firmware
>>>
>>> Tel: +86 021-61166522
>>> iNet: 821-6522
>>>
>>>> -----Original Message-----
>>>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>>>> Sent: Sunday, September 10, 2017 9:52 PM
>>>> To: Shi, Steven <steven.shi@intel.com>; edk2-devel-01 <edk2-
>>>> devel@lists.01.org>
>>>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Dong, Eric <eric.dong@intel.com>;
>> Zeng,
>>>> Star <star.zeng@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>>> Subject: Re: [edk2] [PATCH 0/5] MdeModulePkg: UDF fixes and cleanups
>>>>
>>>> Hi Steven,
>>>>
>>>> On 09/10/17 10:38, Laszlo Ersek wrote:
>>>>> On 09/10/17 06:24, Shi, Steven wrote:
>>>>>> Hi Laszlo,
>>>>>> How could we configure the Qemu and test the UDF driver on OVMF?
>>>>>
>>>>> I guess you would format e.g. a DVD image with UDF, and attach it to
>>>>> QEMU like any other CD-ROM.
>>>>
>>>> I tried to look into this -- I tried several things, but nothing
>>>> produced an UDF image file that, when attached to the VM, would show
>> up
>>>> in the UEFI shell as FSn:
>>>>
>>>> Google returned a bunch of pages, but all I found was:
>>>> - tips that didn't work (see above),
>>>> - confused users (like me) looking for solutions.
>>>>
>>>> So, at the moment, I have no idea how authoring UDF DVD images is
>>>> possible on Linux, so that they'd be recognized in edk2.
>>>>
>>>> (I'm interested in the edk2 UDF driver not because I want to "author"
>>>> UDF DVD images (ISO9660+ElTorito works just fine), but because some
>>>> optical media images that were given to me are formatted UDF-only.
>> They
>>>> can be translated into ISO9660+ElTorito off-line, but that's a chore.)
>>>>
>>>> Thanks,
>>>> Laszlo
>>> _______________________________________________
>>> edk2-devel mailing list
>>> edk2-devel@lists.01.org
>>> https://lists.01.org/mailman/listinfo/edk2-devel
>>>
>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Hi Paulo,
Your fix works for me. The "Null Pointer Use" issue disappears after apply your new assignment patch. Thanks!
Steven Shi
Intel\SSG\STO\UEFI Firmware
Tel: +86 021-61166522
iNet: 821-6522
> -----Original Message-----
> From: Paulo Alcantara [mailto:pcacjr@zytor.com]
> Sent: Monday, September 11, 2017 9:52 PM
> To: Shi, Steven <steven.shi@intel.com>; Laszlo Ersek <lersek@redhat.com>;
> edk2-devel-01 <edk2-devel@lists.01.org>
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Dong, Eric <eric.dong@intel.com>; Zeng,
> Star <star.zeng@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Subject: Re: [edk2] [PATCH 0/5] MdeModulePkg: UDF fixes and cleanups
>
> Hi Steven,
>
> On 11/09/2017 10:07, Shi, Steven wrote:
> > Hi Laszlo,
> > Your code is good. But there is a "Null Pointer Use" issue, which is a
> undefined behavior in C spec, in
> edk2/MdeModulePkg/Universal/Disk/UdfDxe/File.c, line:696, column: 7 and
> line:707, column: 14.
> >
> > Line695:
> > FileIdentifierDesc = PrivFileData->File.FileIdentifierDesc;
> > if (IS_FID_DIRECTORY_FILE (FileIdentifierDesc)) {
> > ...
> >
> > The above FileIdentifierDesc can be NULL if you run a .efi in a Udf partition.
> And the later IS_FID_DIRECTORY_FILE (FileIdentifierDesc) is unsafe if
> FileIdentifierDesc == NULL. You can test it with below extra code which add
> debug output when FileIdentifierDesc == NULL, and check the log after load
> and run something in a Udf partition.
> >
> > +++ b/MdeModulePkg/Universal/Disk/UdfDxe/File.c
> > @@ -693,6 +693,11 @@ UdfSetPosition (
> > PrivFileData = PRIVATE_UDF_FILE_DATA_FROM_THIS (This);
> >
> > FileIdentifierDesc = PrivFileData->File.FileIdentifierDesc;
> > + if (FileIdentifierDesc == NULL) {
> > + DEBUG ((EFI_D_ERROR, "FileIdentifierDesc == NULL!\n"));
> > + return Status;
> > + }
> > +
> > if (IS_FID_DIRECTORY_FILE (FileIdentifierDesc)) {
> > //
> > // If the file handle is a directory, the _only_ position that may be set is
> >
> > I followed the Paulo's suggestion, and found this issue when using the Udf
> partition of Windows 10 Enterprise ISO image on qemu. Below is my qemu
> command:
> > /usr/local/bin/qemu-system-x86_64 -m 5120 -enable-kvm -machine pc-
> q35-2.9 -bios OVMF.fd -serial file:serial.log -cdrom /media/jshi19/My\
> Passport/OSImage/ISO/winserver/en_windows_server_2016_x64_dvd_9327
> 751.iso
> >
> > I've submitted a bug for this issue:
> https://bugzilla.tianocore.org/show_bug.cgi?id=704.
> >
> > I found this issue by edk2 llvm undefined behavior sanitizer, and below is
> the sanitizer output. FYI.
> >
> /home/jshi19/wksp_efi/edk2/MdeModulePkg/Universal/Disk/UdfDxe/File.c,
> line:0x02B8, column:0x0007 ErrorType = NullPointerUse: member access
> within null pointer of type 'UDF_FILE_IDENTIFIER_DESCRIPTOR'
> > ASAN MEMORY ACCESS check fail! __ubsan_handle_type_mismatch_v1 is
> called:
> > UdfSetPosition
> >
> /home/jshi19/wksp_efi/edk2/MdeModulePkg/Universal/Disk/UdfDxe/File.c:
> 696:7
> > EfiShellFindFilesInDir
> >
> /home/jshi19/wksp_efi/edk2/ShellPkg/Application/Shell/ShellProtocol.c:213
> 6:18
> > ShellSearchHandle
> >
> /home/jshi19/wksp_efi/edk2/ShellPkg/Application/Shell/ShellProtocol.c:241
> 3:14
> > EfiShellFindFiles
> >
> /home/jshi19/wksp_efi/edk2/ShellPkg/Application/Shell/ShellProtocol.c:258
> 6:18
> > EfiShellOpenFileList
> >
> /home/jshi19/wksp_efi/edk2/ShellPkg/Application/Shell/ShellProtocol.c:266
> 6:12
> > ShellOpenFileMetaArg
> >
> /home/jshi19/wksp_efi/edk2/ShellPkg/Library/UefiShellLib/UefiShellLib.c:15
> 70:14
> >
> >
> /home/jshi19/wksp_efi/edk2/MdeModulePkg/Universal/Disk/UdfDxe/File.c,
> line:0x02C3, column:0x000E ErrorType = NullPointerUse: member access
> within null pointer of type 'UDF_FILE_IDENTIFIER_DESCRIPTOR'
> > ASAN MEMORY ACCESS check fail! __ubsan_handle_type_mismatch_v1 is
> called:
> > ...
>
> Thank you for catching this bug up. I think this issue is related to
> accessing a File Identifier Descriptor from a root directory file --
> e.g., this should be "FileIdentifierDesc =
> PrivFileData->Root->FileIdentifierDesc;"
>
> Could you please replace the broken assignment with:
>
> FileIdentifierDesc = _FILE(PrivFileData)->FileIdentifierDesc;
> ASSERT (FileIdentifierDesc != NULL);
> ...
>
> Tell me if that worked for you. If so, I could send a formal patch
> fixing this issue later.
>
> (BTW, I do apologize the late replies but I'm only able to work on this
> in my free time)
>
> Thanks,
> Paulo
>
> >
> > Steven Shi
> > Intel\SSG\STO\UEFI Firmware
> >
> > Tel: +86 021-61166522
> > iNet: 821-6522
> >
> >> -----Original Message-----
> >> From: Paulo Alcantara [mailto:pcacjr@zytor.com]
> >> Sent: Sunday, September 10, 2017 11:52 PM
> >> To: Shi, Steven <steven.shi@intel.com>; Laszlo Ersek
> <lersek@redhat.com>;
> >> edk2-devel-01 <edk2-devel@lists.01.org>
> >> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Dong, Eric <eric.dong@intel.com>;
> Zeng,
> >> Star <star.zeng@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >> Subject: Re: [edk2] [PATCH 0/5] MdeModulePkg: UDF fixes and cleanups
> >>
> >>
> >>
> >> On 10/09/2017 11:27, Shi, Steven wrote:
> >>> OK. Does the UDF image you created correctly show up as CD-ROM
> >> content in Linux, e.g Fedora?
> >>
> >> The Fedora image I have (Fedora-Workstation-Live-x86_64-26-1.5.iso)
> does
> >> not contain a valid UDF file system, so you cannot use it for testing.
> >>
> >> To make sure it really doesn't, I used a "Philips UDF Conformance Tool"
> >> that you can grab in [1], and the tool didn't find any valid UDF file
> >> system in it.
> >>
> >> I've been using an unmodified Windows 10 Enterprise image that does
> >> contain a valid UDF bridge disk image (ISO9660+ElTorito+UDF) to perform
> >> my tests.
> >>
> >> Additionally, I use an USB stick that I format it with 'sudo mkudffs -b
> >> 512 --media-type=hd /dev/sdX' and copy some files to it for testing
> >>
> >> BTW, when testing with OVMF AARCH64, I was using my USB stick that
> >> showed up correctly as 'fsX' in UEFI shell, but with the Windows 10
> >> Enterprise ISO image, it didn't. I looked at the logs to see what was
> >> going on and I found out that the emulated CD-ROM drive is reporting a
> >> block size of 512 instead of 2048 -- which seems wrong to me. With
> >> 'qemu-system-x86_64 -cdrom', it reports a block size of 2048.
> >>
> >> The Partition driver is still able to find a valid ElTorito partition
> >> because, regardless the device block size, it always use a logical block
> >> size of 2048 starting at 32K. In UDF, when searching for AVDPs (Anchor
> >> Volume Descriptor Pointers), we search for them at fixed locations: 256,
> >> N - 256, N and 512 -- but, with a block size of 512, the locations
> >> change to 1024, N - 1024, N and 2048 -- thus breaking the volume
> >> recognition sequence.
> >>
> >> For testing the Windows image with a block size of 512, I used the
> >> comformance tool with './udf_test -verbose 60 -blocksize 512
> >> ~/img/win_ent10.iso' and it failed to find a valid UDF file system. But
> >> with a block size of 2048, it worked.
> >>
> >> Is there any reason for reporting a block size of 512 when using
> >> '-cdrom' option in qemu-system-aarch64? Is that a bug? Or am I missing
> >> something here?
> >>
> >> Thanks!
> >> Paulo
> >>
> >> [1] - https://www.lscdweb.com/registered/udf_verifier.html
> >>
> >>>
> >>>
> >>> Steven Shi
> >>> Intel\SSG\STO\UEFI Firmware
> >>>
> >>> Tel: +86 021-61166522
> >>> iNet: 821-6522
> >>>
> >>>> -----Original Message-----
> >>>> From: Laszlo Ersek [mailto:lersek@redhat.com]
> >>>> Sent: Sunday, September 10, 2017 9:52 PM
> >>>> To: Shi, Steven <steven.shi@intel.com>; edk2-devel-01 <edk2-
> >>>> devel@lists.01.org>
> >>>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Dong, Eric <eric.dong@intel.com>;
> >> Zeng,
> >>>> Star <star.zeng@intel.com>; Ard Biesheuvel
> <ard.biesheuvel@linaro.org>
> >>>> Subject: Re: [edk2] [PATCH 0/5] MdeModulePkg: UDF fixes and
> cleanups
> >>>>
> >>>> Hi Steven,
> >>>>
> >>>> On 09/10/17 10:38, Laszlo Ersek wrote:
> >>>>> On 09/10/17 06:24, Shi, Steven wrote:
> >>>>>> Hi Laszlo,
> >>>>>> How could we configure the Qemu and test the UDF driver on OVMF?
> >>>>>
> >>>>> I guess you would format e.g. a DVD image with UDF, and attach it to
> >>>>> QEMU like any other CD-ROM.
> >>>>
> >>>> I tried to look into this -- I tried several things, but nothing
> >>>> produced an UDF image file that, when attached to the VM, would
> show
> >> up
> >>>> in the UEFI shell as FSn:
> >>>>
> >>>> Google returned a bunch of pages, but all I found was:
> >>>> - tips that didn't work (see above),
> >>>> - confused users (like me) looking for solutions.
> >>>>
> >>>> So, at the moment, I have no idea how authoring UDF DVD images is
> >>>> possible on Linux, so that they'd be recognized in edk2.
> >>>>
> >>>> (I'm interested in the edk2 UDF driver not because I want to "author"
> >>>> UDF DVD images (ISO9660+ElTorito works just fine), but because some
> >>>> optical media images that were given to me are formatted UDF-only.
> >> They
> >>>> can be translated into ISO9660+ElTorito off-line, but that's a chore.)
> >>>>
> >>>> Thanks,
> >>>> Laszlo
> >>> _______________________________________________
> >>> edk2-devel mailing list
> >>> edk2-devel@lists.01.org
> >>> https://lists.01.org/mailman/listinfo/edk2-devel
> >>>
> >
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
On 09/10/17 02:12, Laszlo Ersek wrote: > Repo: https://github.com/lersek/edk2.git > Branch: udf_fixes_cleanups > > Patches #2, #3 and #4 are needed (and enough) for me to build OVMF for > IA32 and X64 with clang-3.8, after the UDF introduction. > > Patches #1 and #5 are cleanups that I felt fit before patch #2 and after > patch #4, respectively. > > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> > Cc: Eric Dong <eric.dong@intel.com> > Cc: Paulo Alcantara <pcacjr@zytor.com> > Cc: Ruiyu Ni <ruiyu.ni@intel.com> > Cc: Star Zeng <star.zeng@intel.com> > > Thanks > Laszlo > > Laszlo Ersek (5): > MdeModulePkg/UdfDxe: ASSERT() valid ReadFileInfo Flags for INLINE_DATA > req > MdeModulePkg/UdfDxe: don't return unset Status if INLINE_DATA req > succeeds > MdeModulePkg/UdfDxe: replace zero-init of local variables with > ZeroMem() > MdeModulePkg/PartitionDxe: don't divide 64-bit values with C operators > MdeModulePkg/PartitionDxe: remove always false comparison > > MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c | 9 +++++++-- > MdeModulePkg/Universal/Disk/UdfDxe/File.c | 6 ++++-- > MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c | 5 +++++ > 3 files changed, 16 insertions(+), 4 deletions(-) > Thanks all for the feedback, pushed as commit range c05cae55ebd8..b4e5807d2492. (I didn't change the sizeof / sizeof() stuff -- for one, I didn't want to touch the code on such an urgent push, relative to the posted and tested version.) Thanks Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 12 September 2017 at 03:14, Laszlo Ersek <lersek@redhat.com> wrote:
> On 09/10/17 02:12, Laszlo Ersek wrote:
>> Repo: https://github.com/lersek/edk2.git
>> Branch: udf_fixes_cleanups
>>
>> Patches #2, #3 and #4 are needed (and enough) for me to build OVMF for
>> IA32 and X64 with clang-3.8, after the UDF introduction.
>>
>> Patches #1 and #5 are cleanups that I felt fit before patch #2 and after
>> patch #4, respectively.
>>
>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Cc: Eric Dong <eric.dong@intel.com>
>> Cc: Paulo Alcantara <pcacjr@zytor.com>
>> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
>> Cc: Star Zeng <star.zeng@intel.com>
>>
>> Thanks
>> Laszlo
>>
>> Laszlo Ersek (5):
>> MdeModulePkg/UdfDxe: ASSERT() valid ReadFileInfo Flags for INLINE_DATA
>> req
>> MdeModulePkg/UdfDxe: don't return unset Status if INLINE_DATA req
>> succeeds
>> MdeModulePkg/UdfDxe: replace zero-init of local variables with
>> ZeroMem()
>> MdeModulePkg/PartitionDxe: don't divide 64-bit values with C operators
>> MdeModulePkg/PartitionDxe: remove always false comparison
>>
>> MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c | 9 +++++++--
>> MdeModulePkg/Universal/Disk/UdfDxe/File.c | 6 ++++--
>> MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c | 5 +++++
>> 3 files changed, 16 insertions(+), 4 deletions(-)
>>
>
> Thanks all for the feedback, pushed as commit range
> c05cae55ebd8..b4e5807d2492.
>
> (I didn't change the sizeof / sizeof() stuff -- for one, I didn't want
> to touch the code on such an urgent push, relative to the posted and
> tested version.)
>
Rather unexpectedly, the build is still broken on my CI system
This time, it is GCC 4.8 that complains about uninitialized variables,
most likely false positives again (apologies for the letter soup)
<https://ci.linaro.org/job/leg-virt-tianocore-edk2-upstream/ws/edk2/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c>:
In function 'ReadFile':
<https://ci.linaro.org/job/leg-virt-tianocore-edk2-upstream/ws/edk2/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c>:876:27:
error: 'Status' may be used uninitialized in this function
[-Werror=maybe-uninitialized]
EFI_STATUS Status;
^
"/home/buildslave/srv/toolchain/arm-tc-14.04/bin/arm-linux-gnueabihf-gcc"
-mthumb -mcpu=cortex-a15
-I<https://ci.linaro.org/job/leg-virt-tianocore-edk2-upstream/ws/edk2/ArmVirtPkg/Include>
-g -Os -fshort-wchar -fno-builtin -fno-strict-aliasing -Wall -Werror
-Wno-array-bounds -include AutoGen.h -fno-common -mlittle-endian
-mabi=aapcs -fno-short-enums -funsigned-char -ffunction-sections
-fdata-sections -fomit-frame-pointer -Wno-address -mthumb
-mfloat-abi=soft -fno-pic -fno-pie -fstack-protector
-mword-relocations -Wno-unused-but-set-variable -DMDEPKG_NDEBUG
-DDISABLE_NEW_DEPRECATED_INTERFACES -c -o
<https://ci.linaro.org/job/leg-virt-tianocore-edk2-upstream/ws/edk2/Build/ArmVirtQemu-ARM/RELEASE_GCC48/ARM/OvmfPkg/VirtioNetDxe/VirtioNet/OUTPUT/./SnpSharedHelpers.obj>
-I<https://ci.linaro.org/job/leg-virt-tianocore-edk2-upstream/ws/edk2/OvmfPkg/VirtioNetDxe>
-I<https://ci.linaro.org/job/leg-virt-tianocore-edk2-upstream/ws/edk2/Build/ArmVirtQemu-ARM/RELEASE_GCC48/ARM/OvmfPkg/VirtioNetDxe/VirtioNet/DEBUG>
-I<https://ci.linaro.org/job/leg-virt-tianocore-edk2-upstream/ws/edk2/MdePkg>
-I<https://ci.linaro.org/job/leg-virt-tianocore-edk2-upstream/ws/edk2/MdePkg/Include>
-I<https://ci.linaro.org/job/leg-virt-tianocore-edk2-upstream/ws/edk2/MdePkg/Include/Arm>
-I<https://ci.linaro.org/job/leg-virt-tianocore-edk2-upstream/ws/edk2/OvmfPkg>
-I<https://ci.linaro.org/job/leg-virt-tianocore-edk2-upstream/ws/edk2/OvmfPkg/Include>
<https://ci.linaro.org/job/leg-virt-tianocore-edk2-upstream/ws/edk2/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c>
<https://ci.linaro.org/job/leg-virt-tianocore-edk2-upstream/ws/edk2/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c>:887:27:
error: 'BytesLeft' may be used uninitialized in this function
[-Werror=maybe-uninitialized]
UINT64 BytesLeft;
^
cc1: all warnings being treated as errors
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
On 09/12/17 17:38, Ard Biesheuvel wrote:
> On 12 September 2017 at 03:14, Laszlo Ersek <lersek@redhat.com> wrote:
>> On 09/10/17 02:12, Laszlo Ersek wrote:
>>> Repo: https://github.com/lersek/edk2.git
>>> Branch: udf_fixes_cleanups
>>>
>>> Patches #2, #3 and #4 are needed (and enough) for me to build OVMF for
>>> IA32 and X64 with clang-3.8, after the UDF introduction.
>>>
>>> Patches #1 and #5 are cleanups that I felt fit before patch #2 and after
>>> patch #4, respectively.
>>>
>>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>> Cc: Eric Dong <eric.dong@intel.com>
>>> Cc: Paulo Alcantara <pcacjr@zytor.com>
>>> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
>>> Cc: Star Zeng <star.zeng@intel.com>
>>>
>>> Thanks
>>> Laszlo
>>>
>>> Laszlo Ersek (5):
>>> MdeModulePkg/UdfDxe: ASSERT() valid ReadFileInfo Flags for INLINE_DATA
>>> req
>>> MdeModulePkg/UdfDxe: don't return unset Status if INLINE_DATA req
>>> succeeds
>>> MdeModulePkg/UdfDxe: replace zero-init of local variables with
>>> ZeroMem()
>>> MdeModulePkg/PartitionDxe: don't divide 64-bit values with C operators
>>> MdeModulePkg/PartitionDxe: remove always false comparison
>>>
>>> MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c | 9 +++++++--
>>> MdeModulePkg/Universal/Disk/UdfDxe/File.c | 6 ++++--
>>> MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c | 5 +++++
>>> 3 files changed, 16 insertions(+), 4 deletions(-)
>>>
>>
>> Thanks all for the feedback, pushed as commit range
>> c05cae55ebd8..b4e5807d2492.
>>
>> (I didn't change the sizeof / sizeof() stuff -- for one, I didn't want
>> to touch the code on such an urgent push, relative to the posted and
>> tested version.)
>>
>
> Rather unexpectedly, the build is still broken on my CI system
>
> This time, it is GCC 4.8 that complains about uninitialized variables,
> most likely false positives again (apologies for the letter soup)
>
> <https://ci.linaro.org/job/leg-virt-tianocore-edk2-upstream/ws/edk2/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c>:
> In function 'ReadFile':
> <https://ci.linaro.org/job/leg-virt-tianocore-edk2-upstream/ws/edk2/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c>:876:27:
> error: 'Status' may be used uninitialized in this function
> [-Werror=maybe-uninitialized]
> EFI_STATUS Status;
> ^
This is actually a bug in the code.
In order to explain that, I have to elaborate.
The UDF standard from the OSTA is introduced like this:
The OSTA Universal Disk Format (UDF ® ) specification defines a
subset of the standard ECMA 167 3 rd edition. The primary goal of
the OSTA UDF is to maximize data interchange and minimize the cost
and complexity of implementing ECMA 167.
Which (in retrospect...) means that we have to download ECMA 167 at once:
https://www.ecma-international.org/publications/standards/Ecma-167.htm
OK, so let's look at the code.
First, the ReadFile() function is very hard to read. It is long (~300
lines), but what is actually problematic about it is that it uses "goto"
statements for control flow that is *not* related to error handling.
This makes it painful to track, and it happens to violate the edk2
coding style spec as well:
https://edk2-docs.gitbooks.io/edk-ii-c-coding-standards-specification/content/5_source_files/57_c_programming.html#57-c-programming
5.7.3.8 Goto Statements should not be used (in general)
In almost all cases, it is possible to write the code so that a goto
is not needed. If a goto is used, be ready to defend it during
review.
It is common to use goto for error handling and thus, exiting a
routine in an error case is the only legal use of a goto. A goto
allows the error exit code to be contained in one place in the
routine. This one place reduces software life cycle maintenance
issues, as there can be one copy of error cleanup code per routine.
[...]
Second, in this case it actually pays off to read the function top-down.
- The first switch statement (on "ReadFileInfo->Flags") does not set
Status, but it also doesn't read it or jump to a read site.
- The second switch statement has four case labels (RecordingFlags):
- INLINE_DATA: it sets Status to EFI_SUCCESS
(thanks to my recent patch)
- LONG_ADS_SEQUENCE, SHORT_ADS_SEQUENCE: Status is set very
soon, when we unconditionally call GetAllocationDescriptor().
- EXTENDED_ADS_SEQUENCE: we ASSERT(FALSE), but also set Status to
EFI_UNSUPPORTED
Then, assuming we didn't jump to any of the error labels
(Error_Read_Disk_Blk, Error_Alloc_Buffer_To_Next_Ad, Error_Get_Aed), we
proceed to the Done label, and return Status.
(BTW, I checked all the paths to those error labels, and those *are* fine.)
So how is it possible that we reach Done, and return Status, without
having set Status? It is possible by taking *none* of the branches in
the second switch statement (RecordingFlags). The switch statement does
not have a default label, which also happens to break the coding style:
5.7.3.7 switch Statements
[...]
Always include the default case. It should always end with a break
statement, even if the break is the last thing before the closing
brace.
OK, so how is it possible that none of the case labels match?
"RecordingFlags", i.e. the controlling expression of the switch
statement, is set like this:
RecordingFlags = GET_FE_RECORDING_FLAGS (FileEntryData);
which in turn is #defined as
typedef enum {
SHORT_ADS_SEQUENCE,
LONG_ADS_SEQUENCE,
EXTENDED_ADS_SEQUENCE,
INLINE_DATA
} UDF_FE_RECORDING_FLAGS;
#define GET_FE_RECORDING_FLAGS(_Fe) \
((UDF_FE_RECORDING_FLAGS)((UDF_ICB_TAG *)( \
(UINT8 *)(_Fe) + \
sizeof (UDF_DESCRIPTOR_TAG)))->Flags & 0x07)
So basically we take a bitmask which may have values 0..7 decimal
inclusive (bits 2:0), and then compare it against values 0, 1, 2, 3
only. Because this value comes from an on-disk data structure, i.e. we
don't set it ourselves in the driver, this is an actual bug. A default
label should be added to reject the unsupported cases.
Now, the UDF spec documents this field under
2.3.5 ICB Tag
2.3.5.4 Uint16 Flags
Bits 0-2: These bits specify the type of allocation descriptors used.
Refer to section 2.3.10 on Allocation Descriptors for the guidelines
on choosing which type of allocation descriptor to use.
So we go to 2.3.10:
2.3.10 Allocation Descriptors
[...]
and we find that from the 4 constants listed in the code, the UDF spec
names only two, "Short Allocation Descriptor" (SHORT_ADS_SEQUENCE) and
"Long Allocation Descriptor" (LONG_ADS_SEQUENCE), but the spec doesn't
even provide their numerical values!
This is where ECMA-167 becomes relevant. We have:
14.6 ICB Tag
14.6.8 Flags (RBP 18)
Bit Interpretation
0-2 Shall be interpreted as a 3-bit unsigned binary number as follows.
The value 0 shall mean that Short Allocation Descriptors
(4/14.14.1) are used. The value 1 shall mean that Long Allocation
Descriptors (4/14.14.2) are used. The value 2 shall mean that
Extended Allocation Descriptors (4/14.14.3) are used. The value 3
shall mean that the file shall be treated as though it had exactly
one allocation descriptor describing an extent which starts with
the first byte of the Allocation Descriptors field and has a
length, in bytes, recorded in the Length of Allocation Descriptors
field. The values of 4-7 are reserved for future standardisation.
The enumeration constants in UDF_FE_RECORDING_FLAGS cover the specified
cases, but the reserved values are not checked against in the 2nd switch
statement, under default case label.
This also proves that setting Status at the top of the function, to
EFI_SUCCESS namely, would have been wrong in commit 131fd40ffc34.
> "/home/buildslave/srv/toolchain/arm-tc-14.04/bin/arm-linux-gnueabihf-gcc"
> -mthumb -mcpu=cortex-a15
> -I<https://ci.linaro.org/job/leg-virt-tianocore-edk2-upstream/ws/edk2/ArmVirtPkg/Include>
> -g -Os -fshort-wchar -fno-builtin -fno-strict-aliasing -Wall -Werror
> -Wno-array-bounds -include AutoGen.h -fno-common -mlittle-endian
> -mabi=aapcs -fno-short-enums -funsigned-char -ffunction-sections
> -fdata-sections -fomit-frame-pointer -Wno-address -mthumb
> -mfloat-abi=soft -fno-pic -fno-pie -fstack-protector
> -mword-relocations -Wno-unused-but-set-variable -DMDEPKG_NDEBUG
> -DDISABLE_NEW_DEPRECATED_INTERFACES -c -o
> <https://ci.linaro.org/job/leg-virt-tianocore-edk2-upstream/ws/edk2/Build/ArmVirtQemu-ARM/RELEASE_GCC48/ARM/OvmfPkg/VirtioNetDxe/VirtioNet/OUTPUT/./SnpSharedHelpers.obj>
> -I<https://ci.linaro.org/job/leg-virt-tianocore-edk2-upstream/ws/edk2/OvmfPkg/VirtioNetDxe>
> -I<https://ci.linaro.org/job/leg-virt-tianocore-edk2-upstream/ws/edk2/Build/ArmVirtQemu-ARM/RELEASE_GCC48/ARM/OvmfPkg/VirtioNetDxe/VirtioNet/DEBUG>
> -I<https://ci.linaro.org/job/leg-virt-tianocore-edk2-upstream/ws/edk2/MdePkg>
> -I<https://ci.linaro.org/job/leg-virt-tianocore-edk2-upstream/ws/edk2/MdePkg/Include>
> -I<https://ci.linaro.org/job/leg-virt-tianocore-edk2-upstream/ws/edk2/MdePkg/Include/Arm>
> -I<https://ci.linaro.org/job/leg-virt-tianocore-edk2-upstream/ws/edk2/OvmfPkg>
> -I<https://ci.linaro.org/job/leg-virt-tianocore-edk2-upstream/ws/edk2/OvmfPkg/Include>
> <https://ci.linaro.org/job/leg-virt-tianocore-edk2-upstream/ws/edk2/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c>
> <https://ci.linaro.org/job/leg-virt-tianocore-edk2-upstream/ws/edk2/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c>:887:27:
> error: 'BytesLeft' may be used uninitialized in this function
> [-Werror=maybe-uninitialized]
> UINT64 BytesLeft;
> ^
This is a false positive indeed. There are three reads of "BytesLeft",
and they are all under:
LONG_ADS_SEQUENCE / SHORT_ADS_SEQUENCE
READ_FILE_SEEK_AND_READ
However, BytesLeft is also assigned near the top of the function, under
READ_FILE_SEEK_AND_READ.
(Of course, this analysis is complicated by the fact that we have a
non-error handling label, namely "Skip_File_Seek", just above the first
read location of "BytesLeft". So we have to verify that nothing jumps
there without having set "BytesLeft". Goto statements that aren't used
for error handling / end-of-function cleanup are horrible.)
I'm going to submit two patches for these issues, but that's officially
the end of the resources I can allocate for this driver now. All further
error reports, even build errors, must go into the TianoCore Bugzilla;
no exceptions.
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
On 10 September 2017 at 01:12, Laszlo Ersek <lersek@redhat.com> wrote: > Repo: https://github.com/lersek/edk2.git > Branch: udf_fixes_cleanups > > Patches #2, #3 and #4 are needed (and enough) for me to build OVMF for > IA32 and X64 with clang-3.8, after the UDF introduction. > > Patches #1 and #5 are cleanups that I felt fit before patch #2 and after > patch #4, respectively. > > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> > Cc: Eric Dong <eric.dong@intel.com> > Cc: Paulo Alcantara <pcacjr@zytor.com> > Cc: Ruiyu Ni <ruiyu.ni@intel.com> > Cc: Star Zeng <star.zeng@intel.com> > Series Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 09/09/2017 21:12, Laszlo Ersek wrote: > Repo: https://github.com/lersek/edk2.git > Branch: udf_fixes_cleanups > > Patches #2, #3 and #4 are needed (and enough) for me to build OVMF for > IA32 and X64 with clang-3.8, after the UDF introduction. > > Patches #1 and #5 are cleanups that I felt fit before patch #2 and after > patch #4, respectively. > > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> > Cc: Eric Dong <eric.dong@intel.com> > Cc: Paulo Alcantara <pcacjr@zytor.com> > Cc: Ruiyu Ni <ruiyu.ni@intel.com> > Cc: Star Zeng <star.zeng@intel.com> > > Thanks > Laszlo > > Laszlo Ersek (5): > MdeModulePkg/UdfDxe: ASSERT() valid ReadFileInfo Flags for INLINE_DATA > req > MdeModulePkg/UdfDxe: don't return unset Status if INLINE_DATA req > succeeds > MdeModulePkg/UdfDxe: replace zero-init of local variables with > ZeroMem() > MdeModulePkg/PartitionDxe: don't divide 64-bit values with C operators > MdeModulePkg/PartitionDxe: remove always false comparison > > MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c | 9 +++++++-- > MdeModulePkg/Universal/Disk/UdfDxe/File.c | 6 ++++-- > MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c | 5 +++++ > 3 files changed, 16 insertions(+), 4 deletions(-) > Looks good to me. Tested it with OVMF X64 and AARCH64. Reviewed-by: Paulo Alcantara <pcacjr@zytor.com> Thanks! Paulo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
© 2016 - 2024 Red Hat, Inc.