| Message ID | 1368693288-19594-1-git-send-email-ivan.hu@canonical.com |
|---|---|
| State | Accepted |
| Headers | show |
On 16/05/13 09:34, Ivan Hu wrote: > Signed-off-by: Ivan Hu <ivan.hu@canonical.com> > --- > src/lib/include/fwts_uefi.h | 2 ++ > src/lib/src/fwts_uefi.c | 12 ++++++++++++ > 2 files changed, 14 insertions(+) > > diff --git a/src/lib/include/fwts_uefi.h b/src/lib/include/fwts_uefi.h > index 98eddb0..c0a6ce5 100644 > --- a/src/lib/include/fwts_uefi.h > +++ b/src/lib/include/fwts_uefi.h > @@ -356,4 +356,6 @@ int fwts_uefi_get_variable_names(fwts_list *list); > void fwts_uefi_print_status_info(fwts_framework *fw, const uint64_t status); > char *fwts_uefi_attribute_info(uint32_t attr); > > +bool fwts_uefi_efivars_iface_exist(void); > + > #endif > diff --git a/src/lib/src/fwts_uefi.c b/src/lib/src/fwts_uefi.c > index f8678ab..55308ba 100644 > --- a/src/lib/src/fwts_uefi.c > +++ b/src/lib/src/fwts_uefi.c > @@ -513,3 +513,15 @@ char *fwts_uefi_attribute_info(uint32_t attr) > > return str; > } > + > +/* > + * fwts_uefi_efivars_fs_exist() > + * check the efivar interface existfwts_uefi_get_interface(&path) > + */ > +bool fwts_uefi_efivars_iface_exist(void) > +{ > + char *path; > + > + return (fwts_uefi_get_interface(&path) == UEFI_IFACE_EFIVARS); > + > +} > Should we also be checking for the legacy UEFI_IFACE_SYSFS too? Or how about checking for no interface or failure: { char *path; int ret = fwts_uefi_get_interface(&path); return (ret != UEFI_IFACE_NONE) && (ret != UEFI_IFACE_UNKNOWN); }
On 05/16/2013 05:33 PM, Colin Ian King wrote: > On 16/05/13 09:34, Ivan Hu wrote: >> Signed-off-by: Ivan Hu <ivan.hu@canonical.com> >> --- >> src/lib/include/fwts_uefi.h | 2 ++ >> src/lib/src/fwts_uefi.c | 12 ++++++++++++ >> 2 files changed, 14 insertions(+) >> >> diff --git a/src/lib/include/fwts_uefi.h b/src/lib/include/fwts_uefi.h >> index 98eddb0..c0a6ce5 100644 >> --- a/src/lib/include/fwts_uefi.h >> +++ b/src/lib/include/fwts_uefi.h >> @@ -356,4 +356,6 @@ int fwts_uefi_get_variable_names(fwts_list *list); >> void fwts_uefi_print_status_info(fwts_framework *fw, const uint64_t status); >> char *fwts_uefi_attribute_info(uint32_t attr); >> >> +bool fwts_uefi_efivars_iface_exist(void); >> + >> #endif >> diff --git a/src/lib/src/fwts_uefi.c b/src/lib/src/fwts_uefi.c >> index f8678ab..55308ba 100644 >> --- a/src/lib/src/fwts_uefi.c >> +++ b/src/lib/src/fwts_uefi.c >> @@ -513,3 +513,15 @@ char *fwts_uefi_attribute_info(uint32_t attr) >> >> return str; >> } >> + >> +/* >> + * fwts_uefi_efivars_fs_exist() >> + * check the efivar interface existfwts_uefi_get_interface(&path) >> + */ >> +bool fwts_uefi_efivars_iface_exist(void) >> +{ >> + char *path; >> + >> + return (fwts_uefi_get_interface(&path) == UEFI_IFACE_EFIVARS); >> + >> +} >> > > Should we also be checking for the legacy UEFI_IFACE_SYSFS too? Or how > about checking for no interface or failure: > > { > char *path; > int ret = fwts_uefi_get_interface(&path); > > return (ret != UEFI_IFACE_NONE) && (ret != UEFI_IFACE_UNKNOWN); > } > > The DB, and KEK variables are defined by the UEFI spec and used in UEFI enviroment. I think it should be checked with the efivars interface only, not in the legacy environment. Ivan
On 17/05/13 03:59, IvanHu wrote: > On 05/16/2013 05:33 PM, Colin Ian King wrote: >> On 16/05/13 09:34, Ivan Hu wrote: >>> Signed-off-by: Ivan Hu <ivan.hu@canonical.com> >>> --- >>> src/lib/include/fwts_uefi.h | 2 ++ >>> src/lib/src/fwts_uefi.c | 12 ++++++++++++ >>> 2 files changed, 14 insertions(+) >>> >>> diff --git a/src/lib/include/fwts_uefi.h b/src/lib/include/fwts_uefi.h >>> index 98eddb0..c0a6ce5 100644 >>> --- a/src/lib/include/fwts_uefi.h >>> +++ b/src/lib/include/fwts_uefi.h >>> @@ -356,4 +356,6 @@ int fwts_uefi_get_variable_names(fwts_list *list); >>> void fwts_uefi_print_status_info(fwts_framework *fw, const uint64_t >>> status); >>> char *fwts_uefi_attribute_info(uint32_t attr); >>> >>> +bool fwts_uefi_efivars_iface_exist(void); >>> + >>> #endif >>> diff --git a/src/lib/src/fwts_uefi.c b/src/lib/src/fwts_uefi.c >>> index f8678ab..55308ba 100644 >>> --- a/src/lib/src/fwts_uefi.c >>> +++ b/src/lib/src/fwts_uefi.c >>> @@ -513,3 +513,15 @@ char *fwts_uefi_attribute_info(uint32_t attr) >>> >>> return str; >>> } >>> + >>> +/* >>> + * fwts_uefi_efivars_fs_exist() >>> + * check the efivar interface existfwts_uefi_get_interface(&path) >>> + */ >>> +bool fwts_uefi_efivars_iface_exist(void) >>> +{ >>> + char *path; >>> + >>> + return (fwts_uefi_get_interface(&path) == UEFI_IFACE_EFIVARS); >>> + >>> +} >>> >> >> Should we also be checking for the legacy UEFI_IFACE_SYSFS too? Or how >> about checking for no interface or failure: >> >> { >> char *path; >> int ret = fwts_uefi_get_interface(&path); >> >> return (ret != UEFI_IFACE_NONE) && (ret != UEFI_IFACE_UNKNOWN); >> } >> >> > > The DB, and KEK variables are defined by the UEFI spec and used in UEFI > enviroment. I think it should be checked with the efivars interface > only, not in the legacy environment. The legacy UEFI_IFACE_SYSFS interface is the old kernel UEFI vars interface for older kernels, hence I believe it is required. Colin > > Ivan >
On 05/17/2013 04:46 PM, Colin Ian King wrote: > On 17/05/13 03:59, IvanHu wrote: >> On 05/16/2013 05:33 PM, Colin Ian King wrote: >>> On 16/05/13 09:34, Ivan Hu wrote: >>>> Signed-off-by: Ivan Hu <ivan.hu@canonical.com> >>>> --- >>>> src/lib/include/fwts_uefi.h | 2 ++ >>>> src/lib/src/fwts_uefi.c | 12 ++++++++++++ >>>> 2 files changed, 14 insertions(+) >>>> >>>> diff --git a/src/lib/include/fwts_uefi.h b/src/lib/include/fwts_uefi.h >>>> index 98eddb0..c0a6ce5 100644 >>>> --- a/src/lib/include/fwts_uefi.h >>>> +++ b/src/lib/include/fwts_uefi.h >>>> @@ -356,4 +356,6 @@ int fwts_uefi_get_variable_names(fwts_list *list); >>>> void fwts_uefi_print_status_info(fwts_framework *fw, const uint64_t >>>> status); >>>> char *fwts_uefi_attribute_info(uint32_t attr); >>>> >>>> +bool fwts_uefi_efivars_iface_exist(void); >>>> + >>>> #endif >>>> diff --git a/src/lib/src/fwts_uefi.c b/src/lib/src/fwts_uefi.c >>>> index f8678ab..55308ba 100644 >>>> --- a/src/lib/src/fwts_uefi.c >>>> +++ b/src/lib/src/fwts_uefi.c >>>> @@ -513,3 +513,15 @@ char *fwts_uefi_attribute_info(uint32_t attr) >>>> >>>> return str; >>>> } >>>> + >>>> +/* >>>> + * fwts_uefi_efivars_fs_exist() >>>> + * check the efivar interface existfwts_uefi_get_interface(&path) >>>> + */ >>>> +bool fwts_uefi_efivars_iface_exist(void) >>>> +{ >>>> + char *path; >>>> + >>>> + return (fwts_uefi_get_interface(&path) == UEFI_IFACE_EFIVARS); >>>> + >>>> +} >>>> >>> >>> Should we also be checking for the legacy UEFI_IFACE_SYSFS too? Or how >>> about checking for no interface or failure: >>> >>> { >>> char *path; >>> int ret = fwts_uefi_get_interface(&path); >>> >>> return (ret != UEFI_IFACE_NONE) && (ret != UEFI_IFACE_UNKNOWN); >>> } >>> >>> >> >> The DB, and KEK variables are defined by the UEFI spec and used in UEFI >> enviroment. I think it should be checked with the efivars interface >> only, not in the legacy environment. > > The legacy UEFI_IFACE_SYSFS interface is the old kernel UEFI vars > interface for older kernels, hence I believe it is required. > > Colin > Actually , I found that checking the secure boot variables db and kek via the vars interface will fail. using hexdump and cat for these variables also failed, u@u-HP-Pavilion-m4-Notebook-PC:/sys/firmware$ sudo hexdump efi/vars/db-d719b2cb-3d3a-4596-a3bc-dad00e67656f/data hexdump: efi/vars/db-d719b2cb-3d3a-4596-a3bc-dad00e67656f/data: Input/output error ,but efivars works fine. That's why I add the patches to specify using efivars interface for running the securebootcert test to avoid the unintentional fail from using vars interface. Ivan
On Fri, 17 May, at 06:15:04PM, IvanHu wrote: > Actually , I found that checking the secure boot variables db and > kek via the vars interface will fail. > using hexdump and cat for these variables also failed, > u@u-HP-Pavilion-m4-Notebook-PC:/sys/firmware$ sudo hexdump > efi/vars/db-d719b2cb-3d3a-4596-a3bc-dad00e67656f/data hexdump: > efi/vars/db-d719b2cb-3d3a-4596-a3bc-dad00e67656f/data: Input/output > error > ,but efivars works fine. That's why I add the patches to specify > using efivars interface for running the securebootcert test to avoid > the unintentional fail from using vars interface. The issue you may be running into here is that the old EFI variable sysfs interface cannot handle variables that are larger than 1024 bytes. This is in fact the reason that the efivarfs file system was created - to address this limitation.
On 17/05/13 13:02, Matt Fleming wrote: > On Fri, 17 May, at 06:15:04PM, IvanHu wrote: >> Actually , I found that checking the secure boot variables db and >> kek via the vars interface will fail. >> using hexdump and cat for these variables also failed, >> u@u-HP-Pavilion-m4-Notebook-PC:/sys/firmware$ sudo hexdump >> efi/vars/db-d719b2cb-3d3a-4596-a3bc-dad00e67656f/data hexdump: >> efi/vars/db-d719b2cb-3d3a-4596-a3bc-dad00e67656f/data: Input/output >> error >> ,but efivars works fine. That's why I add the patches to specify >> using efivars interface for running the securebootcert test to avoid >> the unintentional fail from using vars interface. > > The issue you may be running into here is that the old EFI variable > sysfs interface cannot handle variables that are larger than 1024 bytes. > This is in fact the reason that the efivarfs file system was created - > to address this limitation. > Matt, so is it the case that while the old sysfs interface valid, it should probably be avoided?
On Fri, 17 May, at 01:04:39PM, Colin Ian King wrote: > Matt, so is it the case that while the old sysfs interface valid, it > should probably be avoided? Yeah, you should prefer efivarfs over the old sysfs interface because of that 1024-byte limitation. I do expect the sysfs interface to hang around for quite a while, however, because things like efibootmgr use it.
On 16/05/13 09:34, Ivan Hu wrote: > Signed-off-by: Ivan Hu <ivan.hu@canonical.com> > --- > src/lib/include/fwts_uefi.h | 2 ++ > src/lib/src/fwts_uefi.c | 12 ++++++++++++ > 2 files changed, 14 insertions(+) > > diff --git a/src/lib/include/fwts_uefi.h b/src/lib/include/fwts_uefi.h > index 98eddb0..c0a6ce5 100644 > --- a/src/lib/include/fwts_uefi.h > +++ b/src/lib/include/fwts_uefi.h > @@ -356,4 +356,6 @@ int fwts_uefi_get_variable_names(fwts_list *list); > void fwts_uefi_print_status_info(fwts_framework *fw, const uint64_t status); > char *fwts_uefi_attribute_info(uint32_t attr); > > +bool fwts_uefi_efivars_iface_exist(void); > + > #endif > diff --git a/src/lib/src/fwts_uefi.c b/src/lib/src/fwts_uefi.c > index f8678ab..55308ba 100644 > --- a/src/lib/src/fwts_uefi.c > +++ b/src/lib/src/fwts_uefi.c > @@ -513,3 +513,15 @@ char *fwts_uefi_attribute_info(uint32_t attr) > > return str; > } > + > +/* > + * fwts_uefi_efivars_fs_exist() > + * check the efivar interface exist > + */ > +bool fwts_uefi_efivars_iface_exist(void) > +{ > + char *path; > + > + return (fwts_uefi_get_interface(&path) == UEFI_IFACE_EFIVARS); > + > +} > Acked-by: Colin Ian King <colin.king@canonical.com>
On 05/16/2013 04:34 PM, Ivan Hu wrote: > Signed-off-by: Ivan Hu <ivan.hu@canonical.com> > --- > src/lib/include/fwts_uefi.h | 2 ++ > src/lib/src/fwts_uefi.c | 12 ++++++++++++ > 2 files changed, 14 insertions(+) > > diff --git a/src/lib/include/fwts_uefi.h b/src/lib/include/fwts_uefi.h > index 98eddb0..c0a6ce5 100644 > --- a/src/lib/include/fwts_uefi.h > +++ b/src/lib/include/fwts_uefi.h > @@ -356,4 +356,6 @@ int fwts_uefi_get_variable_names(fwts_list *list); > void fwts_uefi_print_status_info(fwts_framework *fw, const uint64_t status); > char *fwts_uefi_attribute_info(uint32_t attr); > > +bool fwts_uefi_efivars_iface_exist(void); > + > #endif > diff --git a/src/lib/src/fwts_uefi.c b/src/lib/src/fwts_uefi.c > index f8678ab..55308ba 100644 > --- a/src/lib/src/fwts_uefi.c > +++ b/src/lib/src/fwts_uefi.c > @@ -513,3 +513,15 @@ char *fwts_uefi_attribute_info(uint32_t attr) > > return str; > } > + > +/* > + * fwts_uefi_efivars_fs_exist() > + * check the efivar interface exist > + */ > +bool fwts_uefi_efivars_iface_exist(void) > +{ > + char *path; > + > + return (fwts_uefi_get_interface(&path) == UEFI_IFACE_EFIVARS); > + > +} > Acked-by: Alex Hung <alex.hung@canonical.com>
diff --git a/src/lib/include/fwts_uefi.h b/src/lib/include/fwts_uefi.h index 98eddb0..c0a6ce5 100644 --- a/src/lib/include/fwts_uefi.h +++ b/src/lib/include/fwts_uefi.h @@ -356,4 +356,6 @@ int fwts_uefi_get_variable_names(fwts_list *list); void fwts_uefi_print_status_info(fwts_framework *fw, const uint64_t status); char *fwts_uefi_attribute_info(uint32_t attr); +bool fwts_uefi_efivars_iface_exist(void); + #endif diff --git a/src/lib/src/fwts_uefi.c b/src/lib/src/fwts_uefi.c index f8678ab..55308ba 100644 --- a/src/lib/src/fwts_uefi.c +++ b/src/lib/src/fwts_uefi.c @@ -513,3 +513,15 @@ char *fwts_uefi_attribute_info(uint32_t attr) return str; } + +/* + * fwts_uefi_efivars_fs_exist() + * check the efivar interface exist + */ +bool fwts_uefi_efivars_iface_exist(void) +{ + char *path; + + return (fwts_uefi_get_interface(&path) == UEFI_IFACE_EFIVARS); + +}
Signed-off-by: Ivan Hu <ivan.hu@canonical.com> --- src/lib/include/fwts_uefi.h | 2 ++ src/lib/src/fwts_uefi.c | 12 ++++++++++++ 2 files changed, 14 insertions(+)