Patchwork uefirtvariable: fix the unchecked return value

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

Comments

Ivan Hu - May 8, 2013, 3:06 a.m.
Coverity CID #997310: Unchecked return value

Need to check the return value for calling function "ioctl".

Signed-off-by: Ivan Hu <ivan.hu@canonical.com>
---
 src/uefi/uefirtvariable/uefirtvariable.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
Colin King - May 8, 2013, 7:07 a.m.
On 08/05/13 04:06, Ivan Hu wrote:
> Coverity CID #997310: Unchecked return value
> 
> Need to check the return value for calling function "ioctl".
> 
> Signed-off-by: Ivan Hu <ivan.hu@canonical.com>
> ---
>  src/uefi/uefirtvariable/uefirtvariable.c |    5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/src/uefi/uefirtvariable/uefirtvariable.c b/src/uefi/uefirtvariable/uefirtvariable.c
> index 16cd240..7c2fc1f 100644
> --- a/src/uefi/uefirtvariable/uefirtvariable.c
> +++ b/src/uefi/uefirtvariable/uefirtvariable.c
> @@ -858,6 +858,7 @@ static int setvariable_invalidattr(
>  	EFI_GUID *gtestguid,
>  	const uint8_t datadiff)
>  {
> +	long ioret;
>  	struct efi_setvariable setvariable;
>  	uint64_t status;
>  	uint64_t dataindex;
> @@ -873,9 +874,9 @@ static int setvariable_invalidattr(
>  	setvariable.Data = data;
>  	setvariable.status = &status;
>  
> -	ioctl(fd, EFI_RUNTIME_SET_VARIABLE, &setvariable);
> +	ioret = ioctl(fd, EFI_RUNTIME_SET_VARIABLE, &setvariable);
>  
> -	if (status == EFI_SUCCESS) {
> +	if ((status == EFI_SUCCESS) && (ioret != -1)) {
>  		fwts_warning(fw,
>  			"After ExitBootServices() is performed, the "
>  			"attributes %" PRIu32 ", "
> 
No entirely sure about this.  What about the case when ioret == -1, is
this an error/failure?  In other tests this is reported as a failure,
where as in this case it seems to be silently ignored.

Colin
Ivan Hu - May 8, 2013, 8:15 a.m.
On 05/08/2013 03:07 PM, Colin Ian King wrote:
> On 08/05/13 04:06, Ivan Hu wrote:
>> Coverity CID #997310: Unchecked return value
>>
>> Need to check the return value for calling function "ioctl".
>>
>> Signed-off-by: Ivan Hu <ivan.hu@canonical.com>
>> ---
>>   src/uefi/uefirtvariable/uefirtvariable.c |    5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/uefi/uefirtvariable/uefirtvariable.c b/src/uefi/uefirtvariable/uefirtvariable.c
>> index 16cd240..7c2fc1f 100644
>> --- a/src/uefi/uefirtvariable/uefirtvariable.c
>> +++ b/src/uefi/uefirtvariable/uefirtvariable.c
>> @@ -858,6 +858,7 @@ static int setvariable_invalidattr(
>>   	EFI_GUID *gtestguid,
>>   	const uint8_t datadiff)
>>   {
>> +	long ioret;
>>   	struct efi_setvariable setvariable;
>>   	uint64_t status;
>>   	uint64_t dataindex;
>> @@ -873,9 +874,9 @@ static int setvariable_invalidattr(
>>   	setvariable.Data = data;
>>   	setvariable.status = &status;
>>
>> -	ioctl(fd, EFI_RUNTIME_SET_VARIABLE, &setvariable);
>> +	ioret = ioctl(fd, EFI_RUNTIME_SET_VARIABLE, &setvariable);
>>
>> -	if (status == EFI_SUCCESS) {
>> +	if ((status == EFI_SUCCESS) && (ioret != -1)) {
>>   		fwts_warning(fw,
>>   			"After ExitBootServices() is performed, the "
>>   			"attributes %" PRIu32 ", "
>>
> No entirely sure about this.  What about the case when ioret == -1, is
> this an error/failure?  In other tests this is reported as a failure,
> where as in this case it seems to be silently ignored.
>
> Colin
>
>
>

In this test case, we do the setvariable with the invalid attributes.
We expects the firmware return a fail for the setvariable, ioret == -1. 
If not, will give an warnings.

Ivan
Colin King - May 8, 2013, 8:28 a.m.
On 08/05/13 04:06, Ivan Hu wrote:
> Coverity CID #997310: Unchecked return value
> 
> Need to check the return value for calling function "ioctl".
> 
> Signed-off-by: Ivan Hu <ivan.hu@canonical.com>
> ---
>  src/uefi/uefirtvariable/uefirtvariable.c |    5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/src/uefi/uefirtvariable/uefirtvariable.c b/src/uefi/uefirtvariable/uefirtvariable.c
> index 16cd240..7c2fc1f 100644
> --- a/src/uefi/uefirtvariable/uefirtvariable.c
> +++ b/src/uefi/uefirtvariable/uefirtvariable.c
> @@ -858,6 +858,7 @@ static int setvariable_invalidattr(
>  	EFI_GUID *gtestguid,
>  	const uint8_t datadiff)
>  {
> +	long ioret;
>  	struct efi_setvariable setvariable;
>  	uint64_t status;
>  	uint64_t dataindex;
> @@ -873,9 +874,9 @@ static int setvariable_invalidattr(
>  	setvariable.Data = data;
>  	setvariable.status = &status;
>  
> -	ioctl(fd, EFI_RUNTIME_SET_VARIABLE, &setvariable);
> +	ioret = ioctl(fd, EFI_RUNTIME_SET_VARIABLE, &setvariable);
>  
> -	if (status == EFI_SUCCESS) {
> +	if ((status == EFI_SUCCESS) && (ioret != -1)) {
>  		fwts_warning(fw,
>  			"After ExitBootServices() is performed, the "
>  			"attributes %" PRIu32 ", "
> 
Acked-by: Colin Ian King <colin.king@canonical.com>
Alex Hung - May 9, 2013, 3:51 a.m.
On 05/08/2013 11:06 AM, Ivan Hu wrote:
> Coverity CID #997310: Unchecked return value
>
> Need to check the return value for calling function "ioctl".
>
> Signed-off-by: Ivan Hu <ivan.hu@canonical.com>
> ---
>   src/uefi/uefirtvariable/uefirtvariable.c |    5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/src/uefi/uefirtvariable/uefirtvariable.c b/src/uefi/uefirtvariable/uefirtvariable.c
> index 16cd240..7c2fc1f 100644
> --- a/src/uefi/uefirtvariable/uefirtvariable.c
> +++ b/src/uefi/uefirtvariable/uefirtvariable.c
> @@ -858,6 +858,7 @@ static int setvariable_invalidattr(
>   	EFI_GUID *gtestguid,
>   	const uint8_t datadiff)
>   {
> +	long ioret;
>   	struct efi_setvariable setvariable;
>   	uint64_t status;
>   	uint64_t dataindex;
> @@ -873,9 +874,9 @@ static int setvariable_invalidattr(
>   	setvariable.Data = data;
>   	setvariable.status = &status;
>
> -	ioctl(fd, EFI_RUNTIME_SET_VARIABLE, &setvariable);
> +	ioret = ioctl(fd, EFI_RUNTIME_SET_VARIABLE, &setvariable);
>
> -	if (status == EFI_SUCCESS) {
> +	if ((status == EFI_SUCCESS) && (ioret != -1)) {
>   		fwts_warning(fw,
>   			"After ExitBootServices() is performed, the "
>   			"attributes %" PRIu32 ", "
>
Acked-by: Alex Hung <alex.hung@canonical.com>

Patch

diff --git a/src/uefi/uefirtvariable/uefirtvariable.c b/src/uefi/uefirtvariable/uefirtvariable.c
index 16cd240..7c2fc1f 100644
--- a/src/uefi/uefirtvariable/uefirtvariable.c
+++ b/src/uefi/uefirtvariable/uefirtvariable.c
@@ -858,6 +858,7 @@  static int setvariable_invalidattr(
 	EFI_GUID *gtestguid,
 	const uint8_t datadiff)
 {
+	long ioret;
 	struct efi_setvariable setvariable;
 	uint64_t status;
 	uint64_t dataindex;
@@ -873,9 +874,9 @@  static int setvariable_invalidattr(
 	setvariable.Data = data;
 	setvariable.status = &status;
 
-	ioctl(fd, EFI_RUNTIME_SET_VARIABLE, &setvariable);
+	ioret = ioctl(fd, EFI_RUNTIME_SET_VARIABLE, &setvariable);
 
-	if (status == EFI_SUCCESS) {
+	if ((status == EFI_SUCCESS) && (ioret != -1)) {
 		fwts_warning(fw,
 			"After ExitBootServices() is performed, the "
 			"attributes %" PRIu32 ", "