Re: [PATCH 0/5] MdeModulePkg: UDF fixes and cleanups

[Laszlo Ersek <lersek@redhat.com>]

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