lkml.org 
[lkml]   [2011]   [Dec]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH 3/4] EFI: Add support for variables longer than 1024 bytes
    On Wed, Dec 14, 2011 at 2:39 PM, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
    > 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.

    Ah. I missed that. Hmm, I wonder if this actually works :)

    >
    >> > +
    >> > +       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.

    Well,l we could *not* support returning all the data field for
    datasize > 1024, and simply truncate the field. We are limited by
    PAGE_SIZE by sysfs here anyway (so we don't really want to have a
    variable size memcpy in efivar_show_raw).
    --
    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/

    \
     
     \ /
      Last update: 2011-12-15 00:01    [W:0.029 / U:121.584 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site