[PATCH] xfs_io: fix memory leak in add_enckey

[PATCH] xfs_io: fix memory leak in add_enckey

[Eric Sandeen]

Invalid arguments to add_enckey will leak the "arg" allocation,
so fix that.

Fixes: ba71de04 ("xfs_io/encrypt: add 'add_enckey' command")
Fixes-coverity-id: 1454644
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

diff --git a/io/encrypt.c b/io/encrypt.c
index 17d61cfb..c6a4e190 100644
--- a/io/encrypt.c
+++ b/io/encrypt.c
@@ -696,6 +696,7 @@ add_enckey_f(int argc, char **argv)
 				goto out;
 			break;
 		default:
+			free(arg);
 			return command_usage(&add_enckey_cmd);
 		}
 	}

Re: [PATCH] xfs_io: fix memory leak in add_enckey

[Bill O'Donnell]

On Thu, Nov 07, 2019 at 10:50:59AM -0600, Eric Sandeen wrote:
> Invalid arguments to add_enckey will leak the "arg" allocation,
> so fix that.
> 
> Fixes: ba71de04 ("xfs_io/encrypt: add 'add_enckey' command")
> Fixes-coverity-id: 1454644
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>

Reviewed-by: Bill O'Donnell <billodo@redhat.com>

> ---
> 
> diff --git a/io/encrypt.c b/io/encrypt.c
> index 17d61cfb..c6a4e190 100644
> --- a/io/encrypt.c
> +++ b/io/encrypt.c
> @@ -696,6 +696,7 @@ add_enckey_f(int argc, char **argv)
>  				goto out;
>  			break;
>  		default:
> +			free(arg);
>  			return command_usage(&add_enckey_cmd);
>  		}
>  	}
> 

Re: [PATCH] xfs_io: fix memory leak in add_enckey

[Eric Biggers]

On Thu, Nov 07, 2019 at 10:50:59AM -0600, Eric Sandeen wrote:
> Invalid arguments to add_enckey will leak the "arg" allocation,
> so fix that.
> 
> Fixes: ba71de04 ("xfs_io/encrypt: add 'add_enckey' command")
> Fixes-coverity-id: 1454644
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
> 
> diff --git a/io/encrypt.c b/io/encrypt.c
> index 17d61cfb..c6a4e190 100644
> --- a/io/encrypt.c
> +++ b/io/encrypt.c
> @@ -696,6 +696,7 @@ add_enckey_f(int argc, char **argv)
>  				goto out;
>  			break;
>  		default:
> +			free(arg);
>  			return command_usage(&add_enckey_cmd);
>  		}
>  	}
> 

The same leak happens later in the function too.  How about this instead:

diff --git a/io/encrypt.c b/io/encrypt.c
index 17d61cfb..de48c50c 100644
--- a/io/encrypt.c
+++ b/io/encrypt.c
@@ -678,6 +678,7 @@ add_enckey_f(int argc, char **argv)
 	int c;
 	struct fscrypt_add_key_arg *arg;
 	ssize_t raw_size;
+	int retval = 0;
 
 	arg = calloc(1, sizeof(*arg) + FSCRYPT_MAX_KEY_SIZE + 1);
 	if (!arg) {
@@ -696,14 +697,17 @@ add_enckey_f(int argc, char **argv)
 				goto out;
 			break;
 		default:
-			return command_usage(&add_enckey_cmd);
+			retval = command_usage(&add_enckey_cmd);
+			goto out;
 		}
 	}
 	argc -= optind;
 	argv += optind;
 
-	if (argc != 0)
-		return command_usage(&add_enckey_cmd);
+	if (argc != 0) {
+		retval = command_usage(&add_enckey_cmd);
+		goto out;
+	}
 
 	raw_size = read_until_limit_or_eof(STDIN_FILENO, arg->raw,
 					   FSCRYPT_MAX_KEY_SIZE + 1);
@@ -732,7 +736,7 @@ add_enckey_f(int argc, char **argv)
 out:
 	memset(arg->raw, 0, FSCRYPT_MAX_KEY_SIZE + 1);
 	free(arg);
-	return 0;
+	return retval;
 }
 
 static int

Re: [PATCH] xfs_io: fix memory leak in add_enckey

[Eric Sandeen]

On 11/7/19 3:46 PM, Eric Biggers wrote:
> On Thu, Nov 07, 2019 at 10:50:59AM -0600, Eric Sandeen wrote:
>> Invalid arguments to add_enckey will leak the "arg" allocation,
>> so fix that.
>>
>> Fixes: ba71de04 ("xfs_io/encrypt: add 'add_enckey' command")
>> Fixes-coverity-id: 1454644
>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>> ---
>>
>> diff --git a/io/encrypt.c b/io/encrypt.c
>> index 17d61cfb..c6a4e190 100644
>> --- a/io/encrypt.c
>> +++ b/io/encrypt.c
>> @@ -696,6 +696,7 @@ add_enckey_f(int argc, char **argv)
>>  				goto out;
>>  			break;
>>  		default:
>> +			free(arg);
>>  			return command_usage(&add_enckey_cmd);
>>  		}
>>  	}
>>
> 
> The same leak happens later in the function too.  How about this instead:

whoops yes it does.  I kind of hate "retval = command_usage" but seeing the
memset of the key on the way out it's probably prudent to have one common
exit point after the function gets started.

Care to send this as a formal patch?

Thanks,
-Eric

> diff --git a/io/encrypt.c b/io/encrypt.c
> index 17d61cfb..de48c50c 100644
> --- a/io/encrypt.c
> +++ b/io/encrypt.c
> @@ -678,6 +678,7 @@ add_enckey_f(int argc, char **argv)
>  	int c;
>  	struct fscrypt_add_key_arg *arg;
>  	ssize_t raw_size;
> +	int retval = 0;
>  
>  	arg = calloc(1, sizeof(*arg) + FSCRYPT_MAX_KEY_SIZE + 1);
>  	if (!arg) {
> @@ -696,14 +697,17 @@ add_enckey_f(int argc, char **argv)
>  				goto out;
>  			break;
>  		default:
> -			return command_usage(&add_enckey_cmd);
> +			retval = command_usage(&add_enckey_cmd);
> +			goto out;
>  		}
>  	}
>  	argc -= optind;
>  	argv += optind;
>  
> -	if (argc != 0)
> -		return command_usage(&add_enckey_cmd);
> +	if (argc != 0) {
> +		retval = command_usage(&add_enckey_cmd);
> +		goto out;
> +	}
>  
>  	raw_size = read_until_limit_or_eof(STDIN_FILENO, arg->raw,
>  					   FSCRYPT_MAX_KEY_SIZE + 1);
> @@ -732,7 +736,7 @@ add_enckey_f(int argc, char **argv)
>  out:
>  	memset(arg->raw, 0, FSCRYPT_MAX_KEY_SIZE + 1);
>  	free(arg);
> -	return 0;
> +	return retval;
>  }
>  
>  static int
> 

Re: [PATCH] xfs_io: fix memory leak in add_enckey

[Eric Sandeen]

On 11/7/19 3:58 PM, Eric Sandeen wrote:
> On 11/7/19 3:46 PM, Eric Biggers wrote:
>> On Thu, Nov 07, 2019 at 10:50:59AM -0600, Eric Sandeen wrote:
>>> Invalid arguments to add_enckey will leak the "arg" allocation,
>>> so fix that.
>>>
>>> Fixes: ba71de04 ("xfs_io/encrypt: add 'add_enckey' command")
>>> Fixes-coverity-id: 1454644
>>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>>> ---
>>>
>>> diff --git a/io/encrypt.c b/io/encrypt.c
>>> index 17d61cfb..c6a4e190 100644
>>> --- a/io/encrypt.c
>>> +++ b/io/encrypt.c
>>> @@ -696,6 +696,7 @@ add_enckey_f(int argc, char **argv)
>>>  				goto out;
>>>  			break;
>>>  		default:
>>> +			free(arg);
>>>  			return command_usage(&add_enckey_cmd);
>>>  		}
>>>  	}
>>>
>>
>> The same leak happens later in the function too.  How about this instead:
> 
> whoops yes it does.  I kind of hate "retval = command_usage" but seeing the
> memset of the key on the way out it's probably prudent to have one common
> exit point after the function gets started.
> 
> Care to send this as a formal patch?

<interprets silence as a "no"> ;)

I'll just incorporate your fixes as an addendum to my patch, then.

-Eric

Re: [PATCH] xfs_io: fix memory leak in add_enckey

[Eric Biggers]

On Mon, Nov 11, 2019 at 09:13:45AM -0600, Eric Sandeen wrote:
> On 11/7/19 3:58 PM, Eric Sandeen wrote:
> > On 11/7/19 3:46 PM, Eric Biggers wrote:
> >> On Thu, Nov 07, 2019 at 10:50:59AM -0600, Eric Sandeen wrote:
> >>> Invalid arguments to add_enckey will leak the "arg" allocation,
> >>> so fix that.
> >>>
> >>> Fixes: ba71de04 ("xfs_io/encrypt: add 'add_enckey' command")
> >>> Fixes-coverity-id: 1454644
> >>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> >>> ---
> >>>
> >>> diff --git a/io/encrypt.c b/io/encrypt.c
> >>> index 17d61cfb..c6a4e190 100644
> >>> --- a/io/encrypt.c
> >>> +++ b/io/encrypt.c
> >>> @@ -696,6 +696,7 @@ add_enckey_f(int argc, char **argv)
> >>>  				goto out;
> >>>  			break;
> >>>  		default:
> >>> +			free(arg);
> >>>  			return command_usage(&add_enckey_cmd);
> >>>  		}
> >>>  	}
> >>>
> >>
> >> The same leak happens later in the function too.  How about this instead:
> > 
> > whoops yes it does.  I kind of hate "retval = command_usage" but seeing the
> > memset of the key on the way out it's probably prudent to have one common
> > exit point after the function gets started.
> > 
> > Care to send this as a formal patch?
> 
> <interprets silence as a "no"> ;)
> 
> I'll just incorporate your fixes as an addendum to my patch, then.
> 
> -Eric

Sorry, I didn't receive this because I was dropped from Cc, and I'm not
currently subscribed to linux-xfs.  The patch you committed looks fine, thanks.

- Eric

Re: [PATCH] xfs_io: fix memory leak in add_enckey

[Eric Sandeen]

On 11/11/19 11:27 AM, Eric Biggers wrote:

...

>>>> The same leak happens later in the function too.  How about this instead:
>>>
>>> whoops yes it does.  I kind of hate "retval = command_usage" but seeing the
>>> memset of the key on the way out it's probably prudent to have one common
>>> exit point after the function gets started.
>>>
>>> Care to send this as a formal patch?
>>
>> <interprets silence as a "no"> ;)
>>
>> I'll just incorporate your fixes as an addendum to my patch, then.
>>
>> -Eric
> 
> Sorry, I didn't receive this because I was dropped from Cc, and I'm not
> currently subscribed to linux-xfs.  The patch you committed looks fine, thanks.

Oh, I'm sorry about that, my mistake.

Thanks,
-Eric

Re: [PATCH] xfs_io: fix memory leak in add_enckey

[Eric Biggers]

On Mon, Nov 11, 2019 at 12:07:27PM -0600, Eric Sandeen wrote:
> > 
> > Sorry, I didn't receive this because I was dropped from Cc, and I'm not
> > currently subscribed to linux-xfs.  The patch you committed looks fine, thanks.
> 
> Oh, I'm sorry about that, my mistake.
> 

And it happened again :-)

For the record, this seems to have been my fault.  My .muttrc was missing
	
	set followup_to = no
	
and at some point I had added mailing list declarations including:

	subscribe .*@vger.kernel.org
	
so Mutt was generating a Mail-Followup-To header excluding me.  I hadn't noticed
this problem earlier because I'm normally subscribed to one of the lists anyway.

- Eric