Ray, I agree that bad code can hang, but good code should process external data without hanging.
It seems at a minimum the release code can check for device path nodes that are too small, and would cause a loop. I would also point out that while there is no limit on the size of any device path, there is a per platform limit of how big a variable can be. So maybe using PcdFlashNvStorageVariableSize as a bounds check on max device path size would be useful? We can add a feature flag to disable this check if needed. Sent from my iPad On Jul 25, 2012, at 9:17 PM, "Ni, Ruiyu" <[email protected]> wrote: > Olivier, > With bad written code, firmware may hung very easily. If the device path is > invalid, even it's handled by the library code. We may still cannot avoid the > hung eventually. > > Thanks, > Ray > > -----Original Message----- > From: Olivier Martin [mailto:[email protected]] > Sent: Thursday, July 19, 2012 6:03 AM > To: [email protected] > Cc: 'Tim Lewis' > Subject: Re: [edk2] MdePkg/UefiDevicePathLib: Potential lock in the loop to > go to the end of a EFI Device Path > > Ray, > I totally agree about the fact the ASSERT() will detect the corrupted Device > Path when the ASSERT is enabled. > But today or with your patch a corrupted EFI Device Path might make hung a > UEFI Firmware ... > Thanks, > Olivier > ________________________________________ > From: Ni, Ruiyu [[email protected]] > Sent: 18 July 2012 11:35 > To: [email protected] > Cc: 'Tim Lewis' > Subject: Re: [edk2] MdePkg/UefiDevicePathLib: Potential lock in the loop > to go to the end of a EFI Device Path > > Olivier, > The reason of the hang can easily be seen due to the ASSERT(). And I intended > to only do the check in DEBUG mode because the PCD for count of device path > node in a device path is hard to define. > The 3rd party UEFI OpROMs can create their own device path. Spec doesn't > restrict the device path node count. > > Thanks, > Ray > > -----Original Message----- > From: Olivier Martin [mailto:[email protected]] > Sent: Thursday, July 12, 2012 5:41 PM > To: [email protected] > Cc: 'Tim Lewis' > Subject: Re: [edk2] MdePkg/UefiDevicePathLib: Potential lock in the loop to > go to the end of a EFI Device Path > > Sorry Ray, I missed your email. > The issue with your patch is you do not handle the error in your code. You > only have 'ASSERT()'. It means if the DevicePath is corrupted your platform > still hang (in this case due to the ASSERT()). > And if you have disabled ASSERT() macro in your DSC file it will still lock. > > Olivier > > -----Original Message----- > From: Ni, Ruiyu [mailto:[email protected]] > Sent: 05 July 2012 04:06 > To: [email protected] > Cc: 'Tim Lewis' > Subject: Re: [edk2] MdePkg/UefiDevicePathLib: Potential lock in the loop to > go to the end of a EFI Device Path > > Olivier, > I read your patch and comes with a refined one. Could you please check > whether it meets your needs? > The patch aligns to the current checking pattern in LinkedList.c and it only > does the check in DEBUG mode as well. > > Thanks, > Ray > > -----Original Message----- > From: Olivier Martin [mailto:[email protected]] > Sent: Thursday, July 05, 2012 2:22 AM > To: [email protected] > Cc: 'Tim Lewis' > Subject: Re: [edk2] MdePkg/UefiDevicePathLib: Potential lock in the loop to > go to the end of a EFI Device Path > > Ray, > Please find the patch I submitted in a different thread of the EDK2 > development mailing-list. > Regards, > Olivier > > > -----Original Message----- > From: Ni, Ruiyu [mailto:[email protected]] > Sent: 21 June 2012 02:32 > To: [email protected] > Subject: Re: [edk2] MdePkg/UefiDevicePathLib: Potential lock in the loop to > go to the end of a EFI Device Path > > Olivier, > I agree with your concern. Do you have any suggestion? > > Thanks, > Ray > > -----Original Message----- > From: Olivier Martin [mailto:[email protected]] > Sent: Wednesday, June 20, 2012 10:03 PM > To: [email protected] > Subject: [edk2] MdePkg/UefiDevicePathLib: Potential lock in the loop to go to > the end of a EFI Device Path > > Hello, > > The loop is this one: > > Start = DevicePath; > while (!IsDevicePathEnd (DevicePath)) { > DevicePath = NextDevicePathNode (DevicePath); } > > This loop exists in different forms in these functions: > GetNextDevicePathInstance (), GetDevicePathSize (), IsDevicePathMultiInstance > (). > > For some reasons, my EFI environment variables have been corrupted. And the > Device Path attached to one of the variables did not make sense anymore. > One of the functions above was scanning for the end of the Device Path > (type: END_DEVICE_PATH_TYPE). Unfortunately, the corrupted EFI Device Path > Node has the length '0'. So the function NextDevicePathNode() always returns > the same Node (and locked the UEFI firmware). > > How should we fix the manipulation of a corrupted Device Path Node ? Should > we introduce a function to check the validity of Device Path (eg: by checking > the length of a Device Path is valid against a maximum size) ? Or should we > modify the loops above to avoid a lock ? > > Regards, > Olivier > > > > > ---------------------------------------------------------------------------- > -- > Live Security Virtual Conference > Exclusive live event will cover all the ways today's security and threat > landscape has changed and how IT managers can respond. Discussions will > include endpoint security, mobile security and the latest in malware threats. > http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/ > _______________________________________________ > edk2-devel mailing list > [email protected] > https://lists.sourceforge.net/lists/listinfo/edk2-devel > > ---------------------------------------------------------------------------- > -- > Live Security Virtual Conference > Exclusive live event will cover all the ways today's security and threat > landscape has changed and how IT managers can respond. Discussions will > include endpoint security, mobile security and the latest in malware threats. > http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/ > _______________________________________________ > edk2-devel mailing list > [email protected] > https://lists.sourceforge.net/lists/listinfo/edk2-devel > > > > > ------------------------------------------------------------------------------ > Live Security Virtual Conference > Exclusive live event will cover all the ways today's security and threat > landscape has changed and how IT managers can respond. Discussions will > include endpoint security, mobile security and the latest in malware threats. > http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/ > _______________________________________________ > edk2-devel mailing list > [email protected] > https://lists.sourceforge.net/lists/listinfo/edk2-devel > > ------------------------------------------------------------------------------ > Live Security Virtual Conference > Exclusive live event will cover all the ways today's security and threat > landscape has changed and how IT managers can respond. Discussions will > include endpoint security, mobile security and the latest in malware threats. > http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/ > _______________________________________________ > edk2-devel mailing list > [email protected] > https://lists.sourceforge.net/lists/listinfo/edk2-devel > > > -- IMPORTANT NOTICE: The contents of this email and any attachments are > confidential and may also be privileged. If you are not the intended > recipient, please notify the sender immediately and do not disclose the > contents to any other person, use it for any purpose, or store or copy the > information in any medium. Thank you. > > > ------------------------------------------------------------------------------ > Live Security Virtual Conference > Exclusive live event will cover all the ways today's security and threat > landscape has changed and how IT managers can respond. Discussions will > include endpoint security, mobile security and the latest in malware threats. > http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/ > _______________________________________________ > edk2-devel mailing list > [email protected] > https://lists.sourceforge.net/lists/listinfo/edk2-devel > > ------------------------------------------------------------------------------ > Live Security Virtual Conference > Exclusive live event will cover all the ways today's security and > threat landscape has changed and how IT managers can respond. Discussions > will include endpoint security, mobile security and the latest in malware > threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/ > _______________________________________________ > edk2-devel mailing list > [email protected] > https://lists.sourceforge.net/lists/listinfo/edk2-devel ------------------------------------------------------------------------------ Live Security Virtual Conference Exclusive live event will cover all the ways today's security and threat landscape has changed and how IT managers can respond. Discussions will include endpoint security, mobile security and the latest in malware threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/ _______________________________________________ edk2-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/edk2-devel
