Re: [PATCH 0/3] UDF partition driver fix

[Paulo Alcantara <pcacjr@zytor.com>]

Hi Jiewen,

On 16/09/2017 20:52, Yao, Jiewen wrote:
> Thank you Paulo, to provide a fix for this driver.

No problem! I'm trying to do the best I can in my very short time.

> 
> I do not have comment for this specific patch. I would defer the review 
> work to Star and Ruiyu.

OK.

> 
> I do have some general question for the new UDF support and I would like 
> to know more detail about the quality level.
> 
> As we are seeing some issues in the new UDF driver, would you please 
> share what test you have done for the UDF support? (Not only for this 
> patch, but also for the UDF partition and UDF file system which are 
> already checked in)

I tested it with a Windows 10 Enterprise ISO image (UDF bridge disk 
image). With it, I could test if ElTorito boot still worked, as well as 
if I could list directory and files, print out file contents, etc. in 
its UDF file system.

I also used a 32GiB USB stick by formatting it with 'mkudffs -b 512 
--media-type=hd /dev/sdX' and copied some files to it and performed some 
basic file operations like listening and reading files/symlinks. I built 
a Linux kernel with EFI stub support inside the UDF file system -- it 
booted fine, however the kernel wasn't able to mount the rootfs being an 
UDF file system.

I tried some Fedora images I have but they aren't UDF bridge disk images 
(e.g. only ISO9660 + ElTorito), so couldn't test with them.

There's also a great tool I used to validate the ISO images and see if 
they are complaint with UDF specification: "Philips UDF Conformance 
Tool". You may find it at 
https://www.lscdweb.com/registered/udf_verifier.html

> 
> I ask this specially, because UDF support (partition and file system) 
> adds a brand new group of external input for the UEFI BIOS. For a long 
> time, we are monitoring all the external input.

Yeah, you're right.

> 
> Per our security model, the external input means the input by an end 
> user. The known external includes but not limited to UEFI image (OROM/OS 
> Loader), Capsule Image, Recovery Image, file system, partition, network 
> packet, variable, etc.

Good to know!

> 
> UDF is a new one, because with the new UDF support, now a malicious user 
> may insert a mal-format UDF CDROM to the system and try to break the 
> system. As such, we need evaluate it.

Sure!

> 
> To be specific, would you please share:
> 
> 1)Which UDF spec these 2 drivers (FS and partition) are following?

I'm following the UDF 2.60 specification.

> 
> I have seen you mentioned the header file follows “(revisions 1.02 
> through 2.60)”. But I am not sure about the driver.
> 
> I found you mentioned:
> 
> Originally the driver was written to support UDF file systems as
> 
> specified by OSTA Universal Disk Format Specification 2.60. However,
> 
> some Windows 10 Enterprise ISO (UDF bridge) images that I tested
> 
> supported a revision of 1.02 thus I had to rework the driver a little
> 
> bit to support such revision as well.
> 
> Do you mean you only support 1.02 and 2.6 in driver, or you support 1.02 
> through 2.6?
> 
> 2)Which UDF function is supported? And more important, which UDF 
> function is NOT supported?

The partition + file system drivers should support only 2.60. However, 
as you can see in this patchset, there is a GetPartitionNumber() in 
PartitionDxe, which checks for the UDF revision (1.02 through 2.60) in 
order to retrieve the correct partition number and Partition Descriptor.

For instance, the Windows 10 Enterprise ISO image, by running it with 
the "Philips UDF Conformance Tool", it says "Final UDF Revision range: 
1.02 only", but partition + driver works fine with it because of that 
revision check in GetPartitionNumber() in PartitionDxe.

Anyways, it's UDF 2.60 revision, officially.

> 
> I have seen “Compliance” section in UDF spec, and it lists some optional 
> feature, such as multi-volume, multi-partition, multisession, file name 
> translation, backward read, backward write, etc. >
> 
> I also have interest to know the support level of current existing 
> CDROM, and existing UDF driver in OS (such as Windows, or Linux). How 
> many optional feature are implemented?

None optional feature has been implemented. Only the mandatory ones.

I will list some known limitations that I could remember:

- Partition types other than 1, will not be supported.

- Extended Allocation Descriptors are not supported. However, the spec 
  mentions: "Only Short and Long Allocation Descriptors shall be 
recorded". IIRC, the 'mkudffs' tool in Linux, it may create Extended 
Allocation Descriptors in directories with thousands of files, and the 
UDF conformance tool reports that the UDF file system created by the 
tool is complaint with UDF revision 2.60.

- Only one Logical Volume and Partition Descriptor is supported. See "2. 
Basic Restrictions & Requirements" in UDF 2.60 specification.
- No write support.

There may be other unsupported features I forgot to mention. Sorry. I 
also need to read the unfriendly ECMA-167 and UDF specs again and check 
the remaining unsupported features.

>
> I ask this, because we want to understand how we declare the support 
> level of this UEFI UDF driver. If we just say we support UDF, then naive 
> people may believe we support everything. J
Yes :-)

> 
> 3)Which compatibility test has been done?
> 
> I am sorry that I cannot find the first version patch. I fund you 
> mentioned Win10 ISO is tried in V2. Any more?
> 
> We would like to know how many existing OS installation CDROM (or any 
> other CDROM) has been tried. Such as Linux (RedHat, Ubuntu, Suse, etc), 
> or Windows (Win7, Win8, Win10)?

Currently, I'm only using a Windows 10 Enterprise ISO image. But I also 
tested it with Windows 8 ISO images as well (in 3 years ago :-) )

Like I said, the Fedora ISO image I have didn't serve because it has no 
UDF file system. Wondering if any other Linux ISO image contains an UDF 
file system.

> 
> The more detail, the better. May a list.
> 
> 4)The last but not least important, which negative test (security test) 
> has been done?

None.

> 
> Have you run some fuzzing test?

No.

> 
> Have you tried a mal-format UDF disc? For example, with bad offset or 
> length?

No.

> 
> Have you test the integer overflow / buffer over flow handing in the code?

No.

> 
> NOTE: we should not use ASSERT to handle such error.
> 
> When I review the code, I found below:
> 
>      Status = ReadFileData (
> 
>        BlockIo,
> 
>        DiskIo,
> 
>        Volume,
> 
>        Parent,
> 
>        PrivFileData->FileSize,
> 
>        &PrivFileData->FilePosition,
> 
>        Buffer,
> 
>        &BufferSizeUint64
> 
>        );
> 
>      ASSERT (BufferSizeUint64 <= MAX_UINTN);
> 
>      *BufferSize = (UINTN)BufferSizeUint64;
> 
> I am not sure if we can use ASSERT (BufferSizeUint64 <= MAX_UINTN);
> 
> Can a malicious user construct a bad UDF and make BufferSizeUint64 > 
> MAX_UINTN?
> 
> Does it might happen? Or never happen?

Laszlo already answered :-)

> 
> We documented Appendix B - EDKII code review top 5 in 
> https://github.com/tianocore-docs/Docs/raw/master/White_Papers/A_Tour_Beyond_BIOS_Security_Design_Guide_in_EDK_II.pdf
> 
> 3 of them are matched in these partition and file system drivers. I 
> quote below:
> 
> ===============================
> 
> *If the code consumes input from an untrusted source or region,*
> 
> Ensure that the input is rigorously and adequately validated.
> 
> *Verify buffer overflow is handled. Avoid integer overflow.*
> 
> Try to use subtraction instead of addition and division instead of 
> multiplication.
> 
> *Verify that ASSERT is used properly.*
> 
> ASSERT is used to catch conditions that should /never /happen. If 
> something /might /happen, use error handling instead. We can use both 
> ASSERT and error handling to facilitate debugging – ASSERT allows for 
> earlier detection and isolation of several classes of issues. [McConnell]
> 
> ===============================
> 
> It is just a reminder. If you are already following that, it will be 
> great. Please let us now.

No, I wasn't. But I will make sure to follow that rigorously next time. 
Very good info. Thanks!

> 
> I take a glance of UDF 2.6 specification, but I do not have chance to 
> read all content at this moment.
> 
> If I asked some stupid question , please feel free to correct me.

Not that I know of. I'm also not an UDF expert. I do enjoy writing 
read-only fs drivers in my free time, that is. :-)

> 
> All in all, we hope to understand the current quality level of the UDF 
> partition support and UDF file system.

Sure. You're completely right by asking such questions. If we really 
want to make proper use of it, the more understanding, testing, 
documentation, the better. Thank you very much for all the questions and 
suggestions. Really appreciate it.

I hope you guys (+ community) to test this implementation, report bugs, 
etc. as much as possible. I wish I could work on it full time and give 
better support for you guys. Unfortunately I can't. But I'll do my best 
in giving you some support.

Thanks!
Paulo

> 
> If you can help to provide the detail, it may help us to evaluate.
> 
> Thank you
> 
> Yao Jiewen
> 
> *From:*edk2-devel [mailto:edk2-devel-bounces@lists.01.org] *On Behalf Of 
> *Laszlo Ersek
> *Sent:* Sunday, September 17, 2017 6:17 AM
> *To:* Paulo Alcantara <pcacjr@zytor.com>
> *Cc:* Ni, Ruiyu <ruiyu.ni@intel.com>; Dong, Eric <eric.dong@intel.com>; 
> edk2-devel@lists.01.org; Gao, Liming <liming.gao@intel.com>; Kinney, 
> Michael D <michael.d.kinney@intel.com>; Zeng, Star <star.zeng@intel.com>
> *Subject:* Re: [edk2] [PATCH 0/3] UDF partition driver fix
> 
> Hi Paulo,
> 
> On 09/16/17 23:36, Paulo Alcantara wrote:
>> This series fixes an UDF issue with Partition driver as discussed in ML:
>> 
>> https://lists.01.org/pipermail/edk2-devel/2017-September/014694.html
>> 
>> Thanks!
>> Paulo
>> 
>> Repo:   https://github.com/pcacjr/edk2.git
>> Branch: udf-partition-fix
>> 
>> Paulo Alcantara (3):
>>   MdePkg: Add UDF volume structure definitions
>>   MdeModulePkg/PartitionDxe: Fix creation of UDF logical partition
>>   MdeModulePkg/UdfDxe: Rework driver to support PartitionDxe changes
>> 
>>  MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c     | 307 +++++++++++-
>>  MdeModulePkg/Universal/Disk/UdfDxe/File.c          |  13 +-
>>  .../Universal/Disk/UdfDxe/FileSystemOperations.c   | 525 ++++++++-------------
>>  MdeModulePkg/Universal/Disk/UdfDxe/Udf.c           |   7 -
>>  MdeModulePkg/Universal/Disk/UdfDxe/Udf.h           |  88 +---
>>  MdePkg/Include/IndustryStandard/Udf.h              |  63 +++
>>  6 files changed, 560 insertions(+), 443 deletions(-)
>> 
> 
> Thank you very much for submitting this patchset quickly. I hope it will
> work out, and we won't need the PartitionExperimentalDxe.inf file!
> 
> I have some trivial process-level suggestions:
> 
> * when submitting a patchset, please collect the Cc: tags from all the
> commit messages, and add them to the cover letter manually. This way
> everybody you CC on at least some of the patches will get the cover
> letter too, presonally.
> 
> This matters because otherwise replies to the blurb will also miss those
> people, personally. (I'm now adding everyone manually.)
> 
> * Because edk2 uses long directory and file names, the diffstats are
> frequently truncated like above (see "..."). You can avoid this if you
> format the patches like this:
> 
>    --stat=1000 --stat-graph-width=20
> 
> this will make the pathname column just as wide as necessary, and will
> also keep the chart to the right reasonably narrow.
> 
> * It's probably best to include a reference to
> <https://bugzilla.tianocore.org/show_bug.cgi?id=707> in the commit
> messages (in particular patch #2).
> 
> * Once you post a patchset for a TianoCore BZ, it's useful to link the
> series (from the mailing list archive) in the BZ itself.
> 
> 
> Regarding the code itself, I don't think I can help here in any sensible
> way. (If UDF support were located under OvmfPkg, I would totally
> consider you the owner of those files, verify your patches for them on a
> formal level only, and if that part was fine, I'd give an Acked-by.)
> 
> Thanks!
> Laszlo
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org <mailto:edk2-devel@lists.01.org>
> https://lists.01.org/mailman/listinfo/edk2-devel
>