| Message ID | 4eb1073f-91fb-a4bc-aae8-d54dc5a6b8aa@redhat.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | xfs_io: fix memory leak in add_enckey | expand |
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); > } > } >
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
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 >
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
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
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
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
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); } }
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> ---