Re: [PATCH v1] SecurityPkg: Fix assert when setting key from FAT formatted eMMC/SD/USB

[Laszlo Ersek <lersek@redhat.com>]

On 07/10/18 02:02, Laszlo Ersek wrote:
> On 07/10/18 00:11, Roman Bacik wrote:

>> +    PathName = AllocateZeroPool (PathLength);
>> +    CopyMem (PathName, ((FILEPATH_DEVICE_PATH*)*FilePath)->PathName,
>> PathLength);
> 
> (3) I think it's not necessary to zero-fill the buffer, we're going to
> overwrite it right after the allocation.
> 
> There's a convenience function for that: AllocateCopyPool().
> 
> (4) The number of bytes is not correct IMO. "PathLength" stands for the
> number of bytes in the entire device path node (FILEPATH_DEVICE_PATH),
> including Header and PathName. So, for getting the number of bytes in
> just PathName, we should subtract the size of Header.
> 
> Presently, we over-read the source buffer; it's not causing problems
> because PathName is NUL-terminated.

Sorry, I was unclear; I meant there were no *observable* problems. The
over-read of the source buffer results in garbage at the end of the
target buffer, which are later not consumed due to the terminating NUL
appearing earlier. Still, we should not over-read the source buffer.

Thanks
Laszlo