| Message ID | 1362520494-8917-2-git-send-email-matt@console-pimps.org |
|---|---|
| State | Rejected |
| Headers | show |
On 05/03/13 21:54, Matt Fleming wrote: > From: Matt Fleming <matt.fleming@intel.com> > > Some firmware implementations update VariableNameSize in > GetNextVariableName() with a value that is larger than the actual > buffer required to hold the VariableName string. This is not > technically a bug, but most implementations do update VariableNameSize > with the value of strlen(VariableName) + 1, so print a warning if a > different value is found. > > Signed-off-by: Matt Fleming <matt.fleming@intel.com> > --- > src/uefi/uefirtvariable/uefirtvariable.c | 74 +++++++++++++++++++++++++++++++- > 1 file changed, 72 insertions(+), 2 deletions(-) > > diff --git a/src/uefi/uefirtvariable/uefirtvariable.c b/src/uefi/uefirtvariable/uefirtvariable.c > index e00b343..177dbf9 100644 > --- a/src/uefi/uefirtvariable/uefirtvariable.c > +++ b/src/uefi/uefirtvariable/uefirtvariable.c > @@ -226,7 +226,7 @@ static bool compare_name(uint16_t *name1, uint16_t *name2) > return ident; > } > > -static int getnextvariable_test(fwts_framework *fw) > +static int getnextvariable_test1(fwts_framework *fw) > { > long ioret; > uint64_t status; > @@ -339,6 +339,72 @@ err_restore_env: > return FWTS_ERROR; > } > > +/* > + * Return true if variablenamesize is the length of the > + * NULL-terminated unicode string, variablename. > + */ > +static bool strlen_valid(uint16_t *variablename, uint64_t variablenamesize) > +{ > + uint64_t len; > + uint16_t c; > + > + for (len = 2; len <= variablenamesize; len += sizeof(c)) { > + c = variablename[(len / sizeof(c)) - 1]; > + if (!c) > + break; > + } > + > + return len == variablenamesize; > +} > + > +static int getnextvariable_test2(fwts_framework *fw) > +{ > + long ioret; > + uint64_t status; > + > + struct efi_getnextvariablename getnextvariablename; > + uint64_t variablenamesize = MAX_DATA_LENGTH; > + uint16_t variablename[MAX_DATA_LENGTH]; > + EFI_GUID vendorguid; > + > + getnextvariablename.VariableNameSize = &variablenamesize; > + getnextvariablename.VariableName = variablename; > + getnextvariablename.VendorGuid = &vendorguid; > + getnextvariablename.status = &status; > + > + /* To start the search, need to pass a Null-terminated string in VariableName */ > + variablename[0] = '\0'; > + while (true) { > + variablenamesize = MAX_DATA_LENGTH; > + ioret = ioctl(fd, EFI_RUNTIME_GET_NEXTVARIABLENAME, &getnextvariablename); > + > + if (ioret == -1) { > + > + /* no next variable was found*/ > + if (*getnextvariablename.status == EFI_NOT_FOUND) > + break; > + > + fwts_failed(fw, LOG_LEVEL_HIGH, "UEFIRuntimeGetNextVariableName", > + "Failed to get next variable name with UEFI runtime service."); > + fwts_uefi_print_status_info(fw, status); > + goto err; > + } > + > + if (!strlen_valid(getnextvariablename.VariableName, > + *getnextvariablename.VariableNameSize)) { > + fwts_warning(fw, "UEFIRuntimeGetNextVariableName " > + "Unexpected variable name size returned."); > + goto err; > + } > + }; > + > + return FWTS_OK; > + > +err: > + > + return FWTS_ERROR; > +} > + > static int setvariable_insertvariable(fwts_framework *fw, uint32_t attributes, uint64_t datasize, > uint16_t *varname, EFI_GUID *gtestguid, uint8_t datadiff) > { > @@ -857,7 +923,11 @@ static int uefirtvariable_test2(fwts_framework *fw) > { > int ret; > > - ret = getnextvariable_test(fw); > + ret = getnextvariable_test1(fw); > + if (ret != FWTS_OK) > + return ret; > + > + ret = getnextvariable_test2(fw); > if (ret != FWTS_OK) > return ret; > > Acked-by: Colin Ian King <colin.king@canonical.com>
On 03/06/2013 05:54 AM, Matt Fleming wrote: > From: Matt Fleming <matt.fleming@intel.com> > > Some firmware implementations update VariableNameSize in > GetNextVariableName() with a value that is larger than the actual > buffer required to hold the VariableName string. This is not > technically a bug, but most implementations do update VariableNameSize > with the value of strlen(VariableName) + 1, so print a warning if a > different value is found. > > Signed-off-by: Matt Fleming <matt.fleming@intel.com> > --- > src/uefi/uefirtvariable/uefirtvariable.c | 74 +++++++++++++++++++++++++++++++- > 1 file changed, 72 insertions(+), 2 deletions(-) > > diff --git a/src/uefi/uefirtvariable/uefirtvariable.c b/src/uefi/uefirtvariable/uefirtvariable.c > index e00b343..177dbf9 100644 > --- a/src/uefi/uefirtvariable/uefirtvariable.c > +++ b/src/uefi/uefirtvariable/uefirtvariable.c > @@ -226,7 +226,7 @@ static bool compare_name(uint16_t *name1, uint16_t *name2) > return ident; > } > > -static int getnextvariable_test(fwts_framework *fw) > +static int getnextvariable_test1(fwts_framework *fw) > { > long ioret; > uint64_t status; > @@ -339,6 +339,72 @@ err_restore_env: > return FWTS_ERROR; > } > > +/* > + * Return true if variablenamesize is the length of the > + * NULL-terminated unicode string, variablename. > + */ > +static bool strlen_valid(uint16_t *variablename, uint64_t variablenamesize) > +{ > + uint64_t len; > + uint16_t c; > + > + for (len = 2; len <= variablenamesize; len += sizeof(c)) { > + c = variablename[(len / sizeof(c)) - 1]; > + if (!c) > + break; > + } > + > + return len == variablenamesize; > +} > + > +static int getnextvariable_test2(fwts_framework *fw) > +{ > + long ioret; > + uint64_t status; > + > + struct efi_getnextvariablename getnextvariablename; > + uint64_t variablenamesize = MAX_DATA_LENGTH; > + uint16_t variablename[MAX_DATA_LENGTH]; > + EFI_GUID vendorguid; > + > + getnextvariablename.VariableNameSize = &variablenamesize; > + getnextvariablename.VariableName = variablename; > + getnextvariablename.VendorGuid = &vendorguid; > + getnextvariablename.status = &status; > + > + /* To start the search, need to pass a Null-terminated string in VariableName */ > + variablename[0] = '\0'; > + while (true) { > + variablenamesize = MAX_DATA_LENGTH; > + ioret = ioctl(fd, EFI_RUNTIME_GET_NEXTVARIABLENAME, &getnextvariablename); > + > + if (ioret == -1) { > + > + /* no next variable was found*/ > + if (*getnextvariablename.status == EFI_NOT_FOUND) > + break; > + > + fwts_failed(fw, LOG_LEVEL_HIGH, "UEFIRuntimeGetNextVariableName", > + "Failed to get next variable name with UEFI runtime service."); > + fwts_uefi_print_status_info(fw, status); > + goto err; > + } > + > + if (!strlen_valid(getnextvariablename.VariableName, > + *getnextvariablename.VariableNameSize)) { > + fwts_warning(fw, "UEFIRuntimeGetNextVariableName " > + "Unexpected variable name size returned."); > + goto err; > + } > + }; > + > + return FWTS_OK; > + > +err: > + > + return FWTS_ERROR; > +} > + > static int setvariable_insertvariable(fwts_framework *fw, uint32_t attributes, uint64_t datasize, > uint16_t *varname, EFI_GUID *gtestguid, uint8_t datadiff) > { > @@ -857,7 +923,11 @@ static int uefirtvariable_test2(fwts_framework *fw) > { > int ret; > > - ret = getnextvariable_test(fw); > + ret = getnextvariable_test1(fw); > + if (ret != FWTS_OK) > + return ret; > + > + ret = getnextvariable_test2(fw); > if (ret != FWTS_OK) > return ret; > > Acked-by: Ivan Hu <ivan.hu@canonical.com>
On Tue, 2013-03-05 at 21:54 +0000, Matt Fleming wrote: > From: Matt Fleming <matt.fleming@intel.com> > > Some firmware implementations update VariableNameSize in > GetNextVariableName() with a value that is larger than the actual > buffer required to hold the VariableName string. This is not > technically a bug, but most implementations do update VariableNameSize > with the value of strlen(VariableName) + 1, so print a warning if a > different value is found. > > Signed-off-by: Matt Fleming <matt.fleming@intel.com> > --- > src/uefi/uefirtvariable/uefirtvariable.c | 74 +++++++++++++++++++++++++++++++- > 1 file changed, 72 insertions(+), 2 deletions(-) Folks, I was fairly certain that there were no implementations in the wild that failed to update VariableNameSize on EFI_SUCCESS, but I've just been informed that there are some. You may or may not want to take this patch, since it is warning about an undocumented behaviour - albeit one that many implementations exhibit.
On 06/03/13 11:27, Matt Fleming wrote: > On Tue, 2013-03-05 at 21:54 +0000, Matt Fleming wrote: >> From: Matt Fleming <matt.fleming@intel.com> >> >> Some firmware implementations update VariableNameSize in >> GetNextVariableName() with a value that is larger than the actual >> buffer required to hold the VariableName string. This is not >> technically a bug, but most implementations do update VariableNameSize >> with the value of strlen(VariableName) + 1, so print a warning if a >> different value is found. >> >> Signed-off-by: Matt Fleming <matt.fleming@intel.com> >> --- >> src/uefi/uefirtvariable/uefirtvariable.c | 74 +++++++++++++++++++++++++++++++- >> 1 file changed, 72 insertions(+), 2 deletions(-) > > Folks, I was fairly certain that there were no implementations in the > wild that failed to update VariableNameSize on EFI_SUCCESS, but I've > just been informed that there are some. Urgh, is this because the spec is a tad ambiguous that we're seeing this in the wild? > > You may or may not want to take this patch, since it is warning about an > undocumented behaviour - albeit one that many implementations exhibit. > One of the remits of fwts is to catch ambiguous behaviour so firmware can be fixed before it gets released. Will catching this kind of behaviour be useful? If so, there seems merit to keeping it. Colin
On Wed, 2013-03-06 at 11:32 +0000, Colin Ian King wrote: > On 06/03/13 11:27, Matt Fleming wrote: > > On Tue, 2013-03-05 at 21:54 +0000, Matt Fleming wrote: > >> From: Matt Fleming <matt.fleming@intel.com> > >> > >> Some firmware implementations update VariableNameSize in > >> GetNextVariableName() with a value that is larger than the actual > >> buffer required to hold the VariableName string. This is not > >> technically a bug, but most implementations do update VariableNameSize > >> with the value of strlen(VariableName) + 1, so print a warning if a > >> different value is found. > >> > >> Signed-off-by: Matt Fleming <matt.fleming@intel.com> > >> --- > >> src/uefi/uefirtvariable/uefirtvariable.c | 74 +++++++++++++++++++++++++++++++- > >> 1 file changed, 72 insertions(+), 2 deletions(-) > > > > Folks, I was fairly certain that there were no implementations in the > > wild that failed to update VariableNameSize on EFI_SUCCESS, but I've > > just been informed that there are some. > > Urgh, is this because the spec is a tad ambiguous that we're seeing this > in the wild? Yeah. The GetNextVariableName() documentation in Section 7.2 of the UEFI 2.3.1 spec doesn't say what should happen to VariableNameSize in the case of EFI_SUCCESS. It only says that it should be updated to the size of the buffer required to hold VariableName in the EFI_BUFFER_TOO_SMALL case. But it's worth pointing out that upstream EDK2 does update VariableNameSize with the size of VariableName on every invocation, which means it's likely that firmware that also has this behaviour is based on EDK2 or some derivative. Any implementation that doesn't is probably of the older, weirder variety. > > > > You may or may not want to take this patch, since it is warning about an > > undocumented behaviour - albeit one that many implementations exhibit. > > > > One of the remits of fwts is to catch ambiguous behaviour so firmware > can be fixed before it gets released. Will catching this kind of > behaviour be useful? If so, there seems merit to keeping it. The bug that this test was designed to catch is one affecting HP machines where VariableNameSize is updated on every invocation, but the updated value is neither the size of VariableName nor the initial value of VaraibleNameSize - it's some value in the middle and I'm not quite sure what it is supposed to represent. I think there's merit in catching that particular issue, but probably not in warning if VariableNameSize isn't updated at all. I'll send a v2 of this patch with those changes.
diff --git a/src/uefi/uefirtvariable/uefirtvariable.c b/src/uefi/uefirtvariable/uefirtvariable.c index e00b343..177dbf9 100644 --- a/src/uefi/uefirtvariable/uefirtvariable.c +++ b/src/uefi/uefirtvariable/uefirtvariable.c @@ -226,7 +226,7 @@ static bool compare_name(uint16_t *name1, uint16_t *name2) return ident; } -static int getnextvariable_test(fwts_framework *fw) +static int getnextvariable_test1(fwts_framework *fw) { long ioret; uint64_t status; @@ -339,6 +339,72 @@ err_restore_env: return FWTS_ERROR; } +/* + * Return true if variablenamesize is the length of the + * NULL-terminated unicode string, variablename. + */ +static bool strlen_valid(uint16_t *variablename, uint64_t variablenamesize) +{ + uint64_t len; + uint16_t c; + + for (len = 2; len <= variablenamesize; len += sizeof(c)) { + c = variablename[(len / sizeof(c)) - 1]; + if (!c) + break; + } + + return len == variablenamesize; +} + +static int getnextvariable_test2(fwts_framework *fw) +{ + long ioret; + uint64_t status; + + struct efi_getnextvariablename getnextvariablename; + uint64_t variablenamesize = MAX_DATA_LENGTH; + uint16_t variablename[MAX_DATA_LENGTH]; + EFI_GUID vendorguid; + + getnextvariablename.VariableNameSize = &variablenamesize; + getnextvariablename.VariableName = variablename; + getnextvariablename.VendorGuid = &vendorguid; + getnextvariablename.status = &status; + + /* To start the search, need to pass a Null-terminated string in VariableName */ + variablename[0] = '\0'; + while (true) { + variablenamesize = MAX_DATA_LENGTH; + ioret = ioctl(fd, EFI_RUNTIME_GET_NEXTVARIABLENAME, &getnextvariablename); + + if (ioret == -1) { + + /* no next variable was found*/ + if (*getnextvariablename.status == EFI_NOT_FOUND) + break; + + fwts_failed(fw, LOG_LEVEL_HIGH, "UEFIRuntimeGetNextVariableName", + "Failed to get next variable name with UEFI runtime service."); + fwts_uefi_print_status_info(fw, status); + goto err; + } + + if (!strlen_valid(getnextvariablename.VariableName, + *getnextvariablename.VariableNameSize)) { + fwts_warning(fw, "UEFIRuntimeGetNextVariableName " + "Unexpected variable name size returned."); + goto err; + } + }; + + return FWTS_OK; + +err: + + return FWTS_ERROR; +} + static int setvariable_insertvariable(fwts_framework *fw, uint32_t attributes, uint64_t datasize, uint16_t *varname, EFI_GUID *gtestguid, uint8_t datadiff) { @@ -857,7 +923,11 @@ static int uefirtvariable_test2(fwts_framework *fw) { int ret; - ret = getnextvariable_test(fw); + ret = getnextvariable_test1(fw); + if (ret != FWTS_OK) + return ret; + + ret = getnextvariable_test2(fw); if (ret != FWTS_OK) return ret;