lkml.org 
[lkml]   [2011]   [Dec]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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/

    \
     
     \ /
      Last update: 2011-12-14 23:43    [W:0.027 / U:390.804 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site