Re: [Edk2 Dev] Issue with the FAT driver

Subject: Re: [Edk2 Dev] Issue with the FAT driver

From: "Andrew J. Fish" <afish@apple.com>

To: dev@edk2.tianocore.org

Date: 2009-02-02 17:56:53

Qing,

When we found this issue it looked like FatOpenVolume() was called.  Then FatCleanupVolume() was called and since the Valid structure was FALSE, it was freed. 

  //
  // If the volume is cleared , remove it.
  // The only time volume be invalidated is in DriverBindingStop.
  //
  if (Volume->Root == NULL && !Volume->Valid) {
    //
    // Free the volume structure
    //
    FatFreeVolume (Volume);
  }

This happened during the InstallMultipleProtocolInterfaces() based on a register protocol notify.  And when we return Valid points to freed memory, that in our case was allocated by some one else and it caused data corruption when the TRUE was written.

  //
  // Install our protocol interfaces on the device's handle
  //
  Status = gBS->InstallMultipleProtocolInterfaces (
                  &Volume->Handle,
                  &gEfiSimpleFileSystemProtocolGuid,
                  &Volume->VolumeInterface,
                  NULL
                  );
  if (EFI_ERROR (Status)) {
    goto Done;
  }
  //
  // Volume installed
  //
  DEBUG ((EFI_D_INIT, "%HInstalled Fat filesystem on %x%N\n", Handle));
  Volume->Valid = TRUE;

Given the locks in the Fat driver, maybe this means the Register Protocol Notify was set to TPL_NOTIFY? I will talk to the developer who saw this issue and check the code. 


Andrew Fish

NOT A CONTRIBUTION! 



On Jan 31, 2009, at 7:06 PM, Huang Qing wrote:

Andrew,
      Thanks a lot for raising this issue. If I recalls correctly, the "Valid" attributes in Volume data structure is used to tag whether the Volume data structure can perform File I/O operations and I kept the same behavior as the original FAT driver. Since I do not know your usage model, could you please provide more details so that I can help to diagnose the issue you met?
 
It looks like we had some code that was doing a register protocol notify on the simple file system protocol and started to open the volume in the notify event. There was a path when opening the volume with Volume->Valid = FALSE caused an issue.
[Qing] The transaction (Install Simple File System Protocol and Set Volume->Valid = TRUE) is protected by FatAcquireLock() and FatReleaseLock(). So there should not be any open volume operation between them. In which TPL level do you open the volume with Volume->Valid = FALSE?
 
There is path through the code where Volume is freed if Volume->Valid == FALSE. So by the time you get to the Volume->Valid = TRUE after the protocol install you are overwriting some one else's memory.
[Qing] As far as I know, once the Volume->Valid is set to be FALSE, it will never be set to be TRUE again. The Volume structure to install protocol and set to Volume->Valid must be a newly allocated data structure. Thats just the assumption that I know but it cannot cover all work flow. Could you please let me know you calling path about file operations? That would be very helpful to root cause the memory corruption issue.
 
 
Thanks & Best regards,
Huang, Qing
 
 
 
-----Original Message-----
From: Andrew J. Fish [mailto:afish@apple.com] 
Sent: Friday, January 30, 2009 9:39 PM
To: dev@edk2.tianocore.org
Subject: [Edk2 Dev] Issue with the FAT driver
 
We saw a bug where it looked like setting the Volume->Valid = TRUE 
after the protocol installation caused an issue.
 
It looks like we had some code that was doing a register protocol 
notify on the simple file system protoocl and started to open the 
volume in the notify event. There was a path when opening the volume 
with Volume->Valid = FALSE caused an issue.
 
There is path through the code where Volume is freed if Volume->Valid 
== FALSE. So by the time you get to the Volume->Valid = TRUE after the 
protocol install you are over  writing some one else's memory.
 
The protocol should be "Valid" prior to installing it, mainly due to 
people doing register protocol notify.
 
Here is the code in the extended fat driver.
 
   //
   // Install our protocol interfaces on the device's handle
   //
   Status = gBS->InstallMultipleProtocolInterfaces (
                   &Volume->Handle,
                   &gEfiSimpleFileSystemProtocolGuid,
                   &Volume->VolumeInterface,
                   NULL
                   );
   if (EFI_ERROR (Status)) {
     goto Done;
   }
   //
   // Volume installed
   //
   DEBUG ((EFI_D_INIT, "%HInstalled Fat filesystem on %x%N\n", Handle));
   Volume->Valid = TRUE;
 
 
Andrew Fish
 
NOT A CONTRIBUTION!
 
------------------------------------------------------
 
To unsubscribe from this discussion, please e-mail [unsubscribeURL]