Patchwork [1/2] lib: fwts_uefi: add the function to check the efivars interface existence

login
register
mail settings
Submitter Ivan Hu
Date May 16, 2013, 8:34 a.m.
Message ID <1368693288-19594-1-git-send-email-ivan.hu@canonical.com>
Download mbox | patch
Permalink /patch/244243/
State Accepted
Headers show

Comments

Ivan Hu - May 16, 2013, 8:34 a.m.
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(+)
Colin King - May 16, 2013, 9:33 a.m.
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);
}
Ivan Hu - May 17, 2013, 2:59 a.m.
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
Colin King - May 17, 2013, 8:46 a.m.
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
>
Ivan Hu - May 17, 2013, 10:15 a.m.
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
Matt Fleming - May 17, 2013, 12:02 p.m.
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.
Colin King - May 17, 2013, 12:04 p.m.
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?
Matt Fleming - May 17, 2013, 12:13 p.m.
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.
Colin King - May 20, 2013, 8:08 a.m.
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>
Alex Hung - May 22, 2013, 9:20 a.m.
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>

Patch

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);
+
+}