Messages in this thread | ![/](/images/icornerl.gif) | | Date | Wed, 14 Dec 2011 22:39:50 +0000 | From | Matthew Garrett <> | Subject | Re: [PATCH 3/4] EFI: Add support for variables longer than 1024 bytes |
| |
On Wed, Dec 14, 2011 at 02:14:27PM -0800, Mike Waychison wrote:
> > static efi_status_t > > -get_var_data_locked(struct efivars *efivars, struct efi_variable *var) > > +get_var_data_locked(struct efivars *efivars, struct extended_efi_variable **var) > > { > > efi_status_t status; > > + unsigned long length; > > + > > + if (!*var) > > + *var = kmalloc(sizeof(struct extended_efi_variable), GFP_KERNEL); > > Aren't we holding a spinlock here?
Good point.
> > + > > + (*var)->header.DataSize = 0; > > + status = efivars->ops->get_variable((*var)->header.VariableName, > > + &(*var)->header.VendorGuid, > > + &(*var)->header.Attributes, > > + &(*var)->header.DataSize, > > + (*var)->Data); > > This doesn't look right. ->Data here is after the Data[1024] buffer > embedded in (*var)->header, and a read into this buffer will corrupt > the heap.
DataSize is 0, so we'll never actually read anything back here.
> > + > > + if (status == EFI_BUFFER_TOO_SMALL) { > > + *var = krealloc(*var, sizeof(struct extended_efi_variable) + > > + (*var)->header.DataSize, GFP_KERNEL); > > + status = efivars->ops->get_variable((*var)->header.VariableName, > > + &(*var)->header.VendorGuid, > > + &(*var)->header.Attributes, > > + &(*var)->header.DataSize, > > + (*var)->Data); > > + } > > + > > + length = ((*var)->header.DataSize < 1024) ? (*var)->header.DataSize : > > + 1024; > > + > > + memcpy(&(*var)->header.Data, &(*var)->Data, length); > > This memcpy clobbers the header.Data with the corrupted data when we > didn't use the second path.
We'll always follow the second path providing there's actually data to read back. If there isn't then length will be 0.
> > + if (count == sizeof(struct efi_variable)) { > > + tmp_var = (struct efi_variable *)buf; > > + new_var = kmalloc(sizeof(struct efi_variable) + > > + tmp_var->DataSize, GFP_KERNEL); > > + memcpy(&new_var->header, tmp_var, sizeof(struct efi_variable)); > > + memcpy(&new_var->Data, tmp_var->Data, tmp_var->DataSize); > > + } else if (count > sizeof(struct efi_variable)) { > > + new_var = (struct extended_efi_variable *)buf; > > + } else { > > return -EINVAL; > > + } > > Ugh. This is difficult to follow, and complicates the memory freeing path :(
Entirely agreed.
> We need to be careful here. The store_raw ABI is broken, in the sense > that the ABI from compat mode differs from that in 32bit mode (there > is a long in the efi_variable structure which changes the offsets). I > don't know how to fix it properly and still maintain proper ABI > compatibility.
True.
> What are your thoughts on _not_ wrapping efi_variable with > extended_efi_variable, and instead just using a > "internal_efi_variable" structure that we copy stuff into/outof. I > think that would make the memory management for dealing with the > different sizes a lot easier to follow.
Hm. I think that'd only work if we expose a new interface. Writes would be easy enough to handle, but reads still need to work for old apps.
-- Matthew Garrett | mjg59@srcf.ucam.org -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| ![\](/images/icornerr.gif) |