| Message ID | 20190328012002.842-1-lsahlber@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | cifs: fix kref underflow in close_shroot() | expand |
Should this be CC:stable? On Wed, Mar 27, 2019 at 8:21 PM Ronnie Sahlberg <lsahlber@redhat.com> wrote: > > Fix a bug where we used to not initialize the cached fid structure at all > in open_shroot() if the open was successful but we did not get a lease. > This would leave the structure uninitialized and later when we close the handle > we would in close_shroot() try to kref_put() an uninitialized refcount. > > Fix this by always initializing this structure if the open was successful > but only do the extra get() if we got a lease. > This extra get() is only used to hold the structure until we get a lease > break from the server at which point we will kref_put() it during lease > processing. > > Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com> > --- > fs/cifs/smb2ops.c | 16 +++++++--------- > 1 file changed, 7 insertions(+), 9 deletions(-) > > diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c > index 1022a3771e14..7cfafac255aa 100644 > --- a/fs/cifs/smb2ops.c > +++ b/fs/cifs/smb2ops.c > @@ -717,20 +717,18 @@ int open_shroot(unsigned int xid, struct cifs_tcon *tcon, struct cifs_fid *pfid) > oparms.fid->mid = le64_to_cpu(o_rsp->sync_hdr.MessageId); > #endif /* CIFS_DEBUG2 */ > > - if (o_rsp->OplockLevel == SMB2_OPLOCK_LEVEL_LEASE) > - oplock = smb2_parse_lease_state(server, o_rsp, > - &oparms.fid->epoch, > - oparms.fid->lease_key); > - else > - goto oshr_exit; > - > - > memcpy(tcon->crfid.fid, pfid, sizeof(struct cifs_fid)); > tcon->crfid.tcon = tcon; > tcon->crfid.is_valid = true; > kref_init(&tcon->crfid.refcount); > - kref_get(&tcon->crfid.refcount); > > + if (o_rsp->OplockLevel == SMB2_OPLOCK_LEVEL_LEASE) { > + kref_get(&tcon->crfid.refcount); > + oplock = smb2_parse_lease_state(server, o_rsp, > + &oparms.fid->epoch, > + oparms.fid->lease_key); > + } else > + goto oshr_exit; > > qi_rsp = (struct smb2_query_info_rsp *)rsp_iov[1].iov_base; > if (le32_to_cpu(qi_rsp->OutputBufferLength) < sizeof(struct smb2_file_all_info)) > -- > 2.13.6 >
Yeah, probably On Thu, Mar 28, 2019 at 3:11 PM Steve French <smfrench@gmail.com> wrote: > > Should this be CC:stable? > > On Wed, Mar 27, 2019 at 8:21 PM Ronnie Sahlberg <lsahlber@redhat.com> wrote: > > > > Fix a bug where we used to not initialize the cached fid structure at all > > in open_shroot() if the open was successful but we did not get a lease. > > This would leave the structure uninitialized and later when we close the handle > > we would in close_shroot() try to kref_put() an uninitialized refcount. > > > > Fix this by always initializing this structure if the open was successful > > but only do the extra get() if we got a lease. > > This extra get() is only used to hold the structure until we get a lease > > break from the server at which point we will kref_put() it during lease > > processing. > > > > Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com> > > --- > > fs/cifs/smb2ops.c | 16 +++++++--------- > > 1 file changed, 7 insertions(+), 9 deletions(-) > > > > diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c > > index 1022a3771e14..7cfafac255aa 100644 > > --- a/fs/cifs/smb2ops.c > > +++ b/fs/cifs/smb2ops.c > > @@ -717,20 +717,18 @@ int open_shroot(unsigned int xid, struct cifs_tcon *tcon, struct cifs_fid *pfid) > > oparms.fid->mid = le64_to_cpu(o_rsp->sync_hdr.MessageId); > > #endif /* CIFS_DEBUG2 */ > > > > - if (o_rsp->OplockLevel == SMB2_OPLOCK_LEVEL_LEASE) > > - oplock = smb2_parse_lease_state(server, o_rsp, > > - &oparms.fid->epoch, > > - oparms.fid->lease_key); > > - else > > - goto oshr_exit; > > - > > - > > memcpy(tcon->crfid.fid, pfid, sizeof(struct cifs_fid)); > > tcon->crfid.tcon = tcon; > > tcon->crfid.is_valid = true; > > kref_init(&tcon->crfid.refcount); > > - kref_get(&tcon->crfid.refcount); > > > > + if (o_rsp->OplockLevel == SMB2_OPLOCK_LEVEL_LEASE) { > > + kref_get(&tcon->crfid.refcount); > > + oplock = smb2_parse_lease_state(server, o_rsp, > > + &oparms.fid->epoch, > > + oparms.fid->lease_key); > > + } else > > + goto oshr_exit; > > > > qi_rsp = (struct smb2_query_info_rsp *)rsp_iov[1].iov_base; > > if (le32_to_cpu(qi_rsp->OutputBufferLength) < sizeof(struct smb2_file_all_info)) > > -- > > 2.13.6 > > > > > -- > Thanks, > > Steve
ср, 27 мар. 2019 г. в 22:27, ronnie sahlberg <ronniesahlberg@gmail.com>: > > Yeah, probably > > On Thu, Mar 28, 2019 at 3:11 PM Steve French <smfrench@gmail.com> wrote: > > > > Should this be CC:stable? > > > > On Wed, Mar 27, 2019 at 8:21 PM Ronnie Sahlberg <lsahlber@redhat.com> wrote: > > > > > > Fix a bug where we used to not initialize the cached fid structure at all > > > in open_shroot() if the open was successful but we did not get a lease. > > > This would leave the structure uninitialized and later when we close the handle > > > we would in close_shroot() try to kref_put() an uninitialized refcount. > > > > > > Fix this by always initializing this structure if the open was successful > > > but only do the extra get() if we got a lease. > > > This extra get() is only used to hold the structure until we get a lease > > > break from the server at which point we will kref_put() it during lease > > > processing. > > > > > > Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com> > > > --- > > > fs/cifs/smb2ops.c | 16 +++++++--------- > > > 1 file changed, 7 insertions(+), 9 deletions(-) > > > > > > diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c > > > index 1022a3771e14..7cfafac255aa 100644 > > > --- a/fs/cifs/smb2ops.c > > > +++ b/fs/cifs/smb2ops.c > > > @@ -717,20 +717,18 @@ int open_shroot(unsigned int xid, struct cifs_tcon *tcon, struct cifs_fid *pfid) > > > oparms.fid->mid = le64_to_cpu(o_rsp->sync_hdr.MessageId); > > > #endif /* CIFS_DEBUG2 */ > > > > > > - if (o_rsp->OplockLevel == SMB2_OPLOCK_LEVEL_LEASE) > > > - oplock = smb2_parse_lease_state(server, o_rsp, > > > - &oparms.fid->epoch, > > > - oparms.fid->lease_key); > > > - else > > > - goto oshr_exit; > > > - > > > - > > > memcpy(tcon->crfid.fid, pfid, sizeof(struct cifs_fid)); > > > tcon->crfid.tcon = tcon; > > > tcon->crfid.is_valid = true; > > > kref_init(&tcon->crfid.refcount); > > > - kref_get(&tcon->crfid.refcount); > > > > > > + if (o_rsp->OplockLevel == SMB2_OPLOCK_LEVEL_LEASE) { > > > + kref_get(&tcon->crfid.refcount); > > > + oplock = smb2_parse_lease_state(server, o_rsp, > > > + &oparms.fid->epoch, > > > + oparms.fid->lease_key); > > > + } else > > > + goto oshr_exit; > > > > > > qi_rsp = (struct smb2_query_info_rsp *)rsp_iov[1].iov_base; > > > if (le32_to_cpu(qi_rsp->OutputBufferLength) < sizeof(struct smb2_file_all_info)) > > > -- > > > 2.13.6 > > > > > > > > > -- > > Thanks, > > > > Steve This part of the fix looks good but there is another piece remaining: rc = smb2_validate_and_copy_iov( le16_to_cpu(qi_rsp->OutputBufferOffset), sizeof(struct smb2_file_all_info), &rsp_iov[1], sizeof(struct smb2_file_all_info), (char *)&tcon->crfid.file_all_info); if (rc) goto oshr_exit; ^^^ here RC is non-zero, but we are returning with 2 references on the handle. The caller will assume that the open failed and doesn't call close_shroot thus leaking one reference. The result of smb2_validate_and_copy_iov only affects file_all_info caching but doesn't affect the handle validity. So, RC needs to be reset to 0 unconditionally here before returning from the function. I checked v5.0 kernel and it doesn't seem it is affected by this bug but it has the problem that it doesn't check whether server granted the lease or not - probably it is not that bad to keep the reference forever for non-leased opens, so leaving it for your choice to fix it or not. -- Best regards, Pavel Shilovsky
diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c index 1022a3771e14..7cfafac255aa 100644 --- a/fs/cifs/smb2ops.c +++ b/fs/cifs/smb2ops.c @@ -717,20 +717,18 @@ int open_shroot(unsigned int xid, struct cifs_tcon *tcon, struct cifs_fid *pfid) oparms.fid->mid = le64_to_cpu(o_rsp->sync_hdr.MessageId); #endif /* CIFS_DEBUG2 */ - if (o_rsp->OplockLevel == SMB2_OPLOCK_LEVEL_LEASE) - oplock = smb2_parse_lease_state(server, o_rsp, - &oparms.fid->epoch, - oparms.fid->lease_key); - else - goto oshr_exit; - - memcpy(tcon->crfid.fid, pfid, sizeof(struct cifs_fid)); tcon->crfid.tcon = tcon; tcon->crfid.is_valid = true; kref_init(&tcon->crfid.refcount); - kref_get(&tcon->crfid.refcount); + if (o_rsp->OplockLevel == SMB2_OPLOCK_LEVEL_LEASE) { + kref_get(&tcon->crfid.refcount); + oplock = smb2_parse_lease_state(server, o_rsp, + &oparms.fid->epoch, + oparms.fid->lease_key); + } else + goto oshr_exit; qi_rsp = (struct smb2_query_info_rsp *)rsp_iov[1].iov_base; if (le32_to_cpu(qi_rsp->OutputBufferLength) < sizeof(struct smb2_file_all_info))
Fix a bug where we used to not initialize the cached fid structure at all in open_shroot() if the open was successful but we did not get a lease. This would leave the structure uninitialized and later when we close the handle we would in close_shroot() try to kref_put() an uninitialized refcount. Fix this by always initializing this structure if the open was successful but only do the extra get() if we got a lease. This extra get() is only used to hold the structure until we get a lease break from the server at which point we will kref_put() it during lease processing. Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com> --- fs/cifs/smb2ops.c | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-)