From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dan Carpenter Date: Tue, 08 Jan 2019 09:42:49 +0000 Subject: Re: [PATCH] cifs: fix memory leak of an allocated cifs_ntsd structure Message-Id: <20190108094249.GF3200@kadam> List-Id: References: <20190107171515.4537-1-colin.king@canonical.com> In-Reply-To: <20190107171515.4537-1-colin.king@canonical.com> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Colin King Cc: Steve French , linux-cifs@vger.kernel.org, samba-technical@lists.samba.org, kernel-janitors@vger.kernel.org, linux-kernel@vger.kernel.org On Mon, Jan 07, 2019 at 05:15:15PM +0000, Colin King wrote: > From: Colin Ian King > > The call to SMB2_queary_acl can allocate memory to pntsd and also > return a failure via a call to SMB2_query_acl (and then query_info). > This occurs when query_info allocates the structure and then in > query_info the call to smb2_validate_and_copy_iov fails. Currently the > failure just returns without kfree'ing pntsd hence causing a memory > leak. Fix this by kfree'ing pntsd before returning. > > Detected by CoverityScan, CID#1457059 ("Resource Leak") > > Fixes: 2f1afe25997f ("cifs: Use smb 2 - 3 and cifsacl mount options getacl functions") > Signed-off-by: Colin Ian King > --- > fs/cifs/smb2ops.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c > index cf7eb891804f..6d71958ad2cb 100644 > --- a/fs/cifs/smb2ops.c > +++ b/fs/cifs/smb2ops.c > @@ -2238,8 +2238,10 @@ get_smb2_acl_by_fid(struct cifs_sb_info *cifs_sb, > cifs_put_tlink(tlink); > > cifs_dbg(FYI, "%s: rc = %d ACL len %d\n", __func__, rc, *pacllen); > - if (rc) > + if (rc) { > + kfree(pntsd); > return ERR_PTR(rc); > + } This is a layering violation. The memory was allocated in query_info() so it should be freed there instead. Also if the kmalloc() fails in query_info() then it should return -ENOMEM instead of success. This only affects code which calls SMB2_query_acl(). There are two callers. You have fixed one but the other is also buggy because we're returning uninitialized memory in get_smb2_acl_by_path(). regards, dan carpenter From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dan Carpenter Subject: Re: [PATCH] cifs: fix memory leak of an allocated cifs_ntsd structure Date: Tue, 8 Jan 2019 12:42:49 +0300 Message-ID: <20190108094249.GF3200@kadam> References: <20190107171515.4537-1-colin.king@canonical.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Steve French , linux-cifs@vger.kernel.org, samba-technical@lists.samba.org, kernel-janitors@vger.kernel.org, linux-kernel@vger.kernel.org To: Colin King Return-path: Content-Disposition: inline In-Reply-To: <20190107171515.4537-1-colin.king@canonical.com> Sender: linux-kernel-owner@vger.kernel.org List-Id: linux-cifs.vger.kernel.org On Mon, Jan 07, 2019 at 05:15:15PM +0000, Colin King wrote: > From: Colin Ian King > > The call to SMB2_queary_acl can allocate memory to pntsd and also > return a failure via a call to SMB2_query_acl (and then query_info). > This occurs when query_info allocates the structure and then in > query_info the call to smb2_validate_and_copy_iov fails. Currently the > failure just returns without kfree'ing pntsd hence causing a memory > leak. Fix this by kfree'ing pntsd before returning. > > Detected by CoverityScan, CID#1457059 ("Resource Leak") > > Fixes: 2f1afe25997f ("cifs: Use smb 2 - 3 and cifsacl mount options getacl functions") > Signed-off-by: Colin Ian King > --- > fs/cifs/smb2ops.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c > index cf7eb891804f..6d71958ad2cb 100644 > --- a/fs/cifs/smb2ops.c > +++ b/fs/cifs/smb2ops.c > @@ -2238,8 +2238,10 @@ get_smb2_acl_by_fid(struct cifs_sb_info *cifs_sb, > cifs_put_tlink(tlink); > > cifs_dbg(FYI, "%s: rc = %d ACL len %d\n", __func__, rc, *pacllen); > - if (rc) > + if (rc) { > + kfree(pntsd); > return ERR_PTR(rc); > + } This is a layering violation. The memory was allocated in query_info() so it should be freed there instead. Also if the kmalloc() fails in query_info() then it should return -ENOMEM instead of success. This only affects code which calls SMB2_query_acl(). There are two callers. You have fixed one but the other is also buggy because we're returning uninitialized memory in get_smb2_acl_by_path(). regards, dan carpenter