diff mbox series

cifs: fix strcat buffer overflow in smb21_set_oplock_level()

Message ID 1557155792-2703-1-git-send-email-kernel@probst.it (mailing list archive)
State New, archived
Headers show
Series cifs: fix strcat buffer overflow in smb21_set_oplock_level() | expand

Commit Message

Christoph Probst May 6, 2019, 3:16 p.m. UTC 
Change strcat to strcpy in the "None" case as it is never valid to append
"None" to any other message. It may also overflow char message[5], in a
race condition on cinode if cinode->oplock is unset by another thread
after "RHW" or "RH" had been written to message.

Signed-off-by: Christoph Probst <kernel@probst.it>
---
 fs/cifs/smb2ops.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Steve French May 6, 2019, 4:53 p.m. UTC | #1 
I think strcpy is clearer - but I don't think it can overflow since if
R, W or W were written to "message" then cinode->oplock would be
non-zero so we would never strcap "None"

On Mon, May 6, 2019 at 10:26 AM Christoph Probst <kernel@probst.it> wrote:
>
> Change strcat to strcpy in the "None" case as it is never valid to append
> "None" to any other message. It may also overflow char message[5], in a
> race condition on cinode if cinode->oplock is unset by another thread
> after "RHW" or "RH" had been written to message.
>
> Signed-off-by: Christoph Probst <kernel@probst.it>
> ---
>  fs/cifs/smb2ops.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
> index c36ff0d..5fd5567 100644
> --- a/fs/cifs/smb2ops.c
> +++ b/fs/cifs/smb2ops.c
> @@ -2936,7 +2936,7 @@ smb21_set_oplock_level(struct cifsInodeInfo *cinode, __u32 oplock,
>                 strcat(message, "W");
>         }
>         if (!cinode->oplock)
> -               strcat(message, "None");
> +               strcpy(message, "None");
>         cifs_dbg(FYI, "%s Lease granted on inode %p\n", message,
>                  &cinode->vfs_inode);
>  }
> --
> 2.1.4
>
Jeremy Allison May 6, 2019, 4:56 p.m. UTC | #2 
On Mon, May 06, 2019 at 11:53:44AM -0500, Steve French via samba-technical wrote:
> I think strcpy is clearer - but I don't think it can overflow since if
> R, W or W were written to "message" then cinode->oplock would be
> non-zero so we would never strcap "None"

Ahem. In Samba we have :

lib/util/safe_string.h:#define strcpy(dest,src) __ERROR__XX__NEVER_USE_STRCPY___;

Maybe you should do likewise :-).

> On Mon, May 6, 2019 at 10:26 AM Christoph Probst <kernel@probst.it> wrote:
> >
> > Change strcat to strcpy in the "None" case as it is never valid to append
> > "None" to any other message. It may also overflow char message[5], in a
> > race condition on cinode if cinode->oplock is unset by another thread
> > after "RHW" or "RH" had been written to message.
> >
> > Signed-off-by: Christoph Probst <kernel@probst.it>
> > ---
> >  fs/cifs/smb2ops.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
> > index c36ff0d..5fd5567 100644
> > --- a/fs/cifs/smb2ops.c
> > +++ b/fs/cifs/smb2ops.c
> > @@ -2936,7 +2936,7 @@ smb21_set_oplock_level(struct cifsInodeInfo *cinode, __u32 oplock,
> >                 strcat(message, "W");
> >         }
> >         if (!cinode->oplock)
> > -               strcat(message, "None");
> > +               strcpy(message, "None");
> >         cifs_dbg(FYI, "%s Lease granted on inode %p\n", message,
> >                  &cinode->vfs_inode);
> >  }
> > --
> > 2.1.4
> >
> 
> 
> -- 
> Thanks,
> 
> Steve
>
Steve French May 6, 2019, 5:02 p.m. UTC | #3 
We could always switch it to strncpy :)

In any case - he is correct, it is better than what was there because
we should not strcat unless the array were locked across the whole
function

On Mon, May 6, 2019 at 11:57 AM Jeremy Allison <jra@samba.org> wrote:
>
> On Mon, May 06, 2019 at 11:53:44AM -0500, Steve French via samba-technical wrote:
> > I think strcpy is clearer - but I don't think it can overflow since if
> > R, W or W were written to "message" then cinode->oplock would be
> > non-zero so we would never strcap "None"
>
> Ahem. In Samba we have :
>
> lib/util/safe_string.h:#define strcpy(dest,src) __ERROR__XX__NEVER_USE_STRCPY___;
>
> Maybe you should do likewise :-).
>
> > On Mon, May 6, 2019 at 10:26 AM Christoph Probst <kernel@probst.it> wrote:
> > >
> > > Change strcat to strcpy in the "None" case as it is never valid to append
> > > "None" to any other message. It may also overflow char message[5], in a
> > > race condition on cinode if cinode->oplock is unset by another thread
> > > after "RHW" or "RH" had been written to message.
> > >
> > > Signed-off-by: Christoph Probst <kernel@probst.it>
> > > ---
> > >  fs/cifs/smb2ops.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
> > > index c36ff0d..5fd5567 100644
> > > --- a/fs/cifs/smb2ops.c
> > > +++ b/fs/cifs/smb2ops.c
> > > @@ -2936,7 +2936,7 @@ smb21_set_oplock_level(struct cifsInodeInfo *cinode, __u32 oplock,
> > >                 strcat(message, "W");
> > >         }
> > >         if (!cinode->oplock)
> > > -               strcat(message, "None");
> > > +               strcpy(message, "None");
> > >         cifs_dbg(FYI, "%s Lease granted on inode %p\n", message,
> > >                  &cinode->vfs_inode);
> > >  }
> > > --
> > > 2.1.4
> > >
> >
> >
> > --
> > Thanks,
> >
> > Steve
> >
Pavel Shilovsky May 6, 2019, 7:03 p.m. UTC | #4 
The patch itself is fine but I think we have a bigger problem here:

3052 >-------cinode->oplock = 0;

here we reset oplock level of the inode, so forcing all the IO go to the server.

3053 >-------if (oplock & SMB2_LEASE_READ_CACHING_HE) {
3054 >------->-------cinode->oplock |= CIFS_CACHE_READ_FLG;
3055 >------->-------strcat(message, "R");
3056 >-------}
3057 >-------if (oplock & SMB2_LEASE_HANDLE_CACHING_HE) {
3058 >------->-------cinode->oplock |= CIFS_CACHE_HANDLE_FLG;
3059 >------->-------strcat(message, "H");
3060 >-------}
3061 >-------if (oplock & SMB2_LEASE_WRITE_CACHING_HE) {
3062 >------->-------cinode->oplock |= CIFS_CACHE_WRITE_FLG;

this and 3 above cases are all racy with other threads opening the
same file and the oplock break thread.

3063 >------->-------strcat(message, "W");
3064 >-------}
3065 >-------if (!cinode->oplock)
3066 >------->-------strcat(message, "None");
3067 >-------cifs_dbg(FYI, "%s Lease granted on inode %p\n", message,
3068 >------->------- &cinode->vfs_inode);

Besides the fix in the patch I would temporarily suggest to not touch
cinode->oplock more than once in this function - have a local variable
cifs_oplock which accumulates flags and set cinode->oplock at the very
end. It reduce raciness a little bit but this code requires much more
thinking about proper locking I guess.

Best regards,
Pavel Shilovskiy

пн, 6 мая 2019 г. в 10:02, Steve French via samba-technical
<samba-technical@lists.samba.org>:
>
> We could always switch it to strncpy :)
>
> In any case - he is correct, it is better than what was there because
> we should not strcat unless the array were locked across the whole
> function
>
> On Mon, May 6, 2019 at 11:57 AM Jeremy Allison <jra@samba.org> wrote:
> >
> > On Mon, May 06, 2019 at 11:53:44AM -0500, Steve French via samba-technical wrote:
> > > I think strcpy is clearer - but I don't think it can overflow since if
> > > R, W or W were written to "message" then cinode->oplock would be
> > > non-zero so we would never strcap "None"
> >
> > Ahem. In Samba we have :
> >
> > lib/util/safe_string.h:#define strcpy(dest,src) __ERROR__XX__NEVER_USE_STRCPY___;
> >
> > Maybe you should do likewise :-).
> >
> > > On Mon, May 6, 2019 at 10:26 AM Christoph Probst <kernel@probst.it> wrote:
> > > >
> > > > Change strcat to strcpy in the "None" case as it is never valid to append
> > > > "None" to any other message. It may also overflow char message[5], in a
> > > > race condition on cinode if cinode->oplock is unset by another thread
> > > > after "RHW" or "RH" had been written to message.
> > > >
> > > > Signed-off-by: Christoph Probst <kernel@probst.it>
> > > > ---
> > > >  fs/cifs/smb2ops.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
> > > > index c36ff0d..5fd5567 100644
> > > > --- a/fs/cifs/smb2ops.c
> > > > +++ b/fs/cifs/smb2ops.c
> > > > @@ -2936,7 +2936,7 @@ smb21_set_oplock_level(struct cifsInodeInfo *cinode, __u32 oplock,
> > > >                 strcat(message, "W");
> > > >         }
> > > >         if (!cinode->oplock)
> > > > -               strcat(message, "None");
> > > > +               strcpy(message, "None");
> > > >         cifs_dbg(FYI, "%s Lease granted on inode %p\n", message,
> > > >                  &cinode->vfs_inode);
> > > >  }
> > > > --
> > > > 2.1.4
> > > >
> > >
> > >
> > > --
> > > Thanks,
> > >
> > > Steve
> > >
>
>
>
> --
> Thanks,
>
> Steve
>
Steve French May 6, 2019, 9:18 p.m. UTC | #5 
On Mon, May 6, 2019 at 2:03 PM Pavel Shilovsky
<pavel.shilovsky@gmail.com> wrote:
>
> The patch itself is fine but I think we have a bigger problem here:

Good point.  Perhaps make update to the same patch to include both changes

> 3052 >-------cinode->oplock = 0;
>
> here we reset oplock level of the inode, so forcing all the IO go to the server.
>
> 3053 >-------if (oplock & SMB2_LEASE_READ_CACHING_HE) {
> 3054 >------->-------cinode->oplock |= CIFS_CACHE_READ_FLG;
> 3055 >------->-------strcat(message, "R");
> 3056 >-------}
> 3057 >-------if (oplock & SMB2_LEASE_HANDLE_CACHING_HE) {
> 3058 >------->-------cinode->oplock |= CIFS_CACHE_HANDLE_FLG;
> 3059 >------->-------strcat(message, "H");
> 3060 >-------}
> 3061 >-------if (oplock & SMB2_LEASE_WRITE_CACHING_HE) {
> 3062 >------->-------cinode->oplock |= CIFS_CACHE_WRITE_FLG;
>
> this and 3 above cases are all racy with other threads opening the
> same file and the oplock break thread.
>
> 3063 >------->-------strcat(message, "W");
> 3064 >-------}
> 3065 >-------if (!cinode->oplock)
> 3066 >------->-------strcat(message, "None");
> 3067 >-------cifs_dbg(FYI, "%s Lease granted on inode %p\n", message,
> 3068 >------->------- &cinode->vfs_inode);
>
> Besides the fix in the patch I would temporarily suggest to not touch
> cinode->oplock more than once in this function - have a local variable
> cifs_oplock which accumulates flags and set cinode->oplock at the very
> end. It reduce raciness a little bit but this code requires much more
> thinking about proper locking I guess.
>
> Best regards,
> Pavel Shilovskiy
>
> пн, 6 мая 2019 г. в 10:02, Steve French via samba-technical
> <samba-technical@lists.samba.org>:
> >
> > We could always switch it to strncpy :)
> >
> > In any case - he is correct, it is better than what was there because
> > we should not strcat unless the array were locked across the whole
> > function
> >
> > On Mon, May 6, 2019 at 11:57 AM Jeremy Allison <jra@samba.org> wrote:
> > >
> > > On Mon, May 06, 2019 at 11:53:44AM -0500, Steve French via samba-technical wrote:
> > > > I think strcpy is clearer - but I don't think it can overflow since if
> > > > R, W or W were written to "message" then cinode->oplock would be
> > > > non-zero so we would never strcap "None"
> > >
> > > Ahem. In Samba we have :
> > >
> > > lib/util/safe_string.h:#define strcpy(dest,src) __ERROR__XX__NEVER_USE_STRCPY___;
> > >
> > > Maybe you should do likewise :-).
> > >
> > > > On Mon, May 6, 2019 at 10:26 AM Christoph Probst <kernel@probst.it> wrote:
> > > > >
> > > > > Change strcat to strcpy in the "None" case as it is never valid to append
> > > > > "None" to any other message. It may also overflow char message[5], in a
> > > > > race condition on cinode if cinode->oplock is unset by another thread
> > > > > after "RHW" or "RH" had been written to message.
> > > > >
> > > > > Signed-off-by: Christoph Probst <kernel@probst.it>
> > > > > ---
> > > > >  fs/cifs/smb2ops.c | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
> > > > > index c36ff0d..5fd5567 100644
> > > > > --- a/fs/cifs/smb2ops.c
> > > > > +++ b/fs/cifs/smb2ops.c
> > > > > @@ -2936,7 +2936,7 @@ smb21_set_oplock_level(struct cifsInodeInfo *cinode, __u32 oplock,
> > > > >                 strcat(message, "W");
> > > > >         }
> > > > >         if (!cinode->oplock)
> > > > > -               strcat(message, "None");
> > > > > +               strcpy(message, "None");
> > > > >         cifs_dbg(FYI, "%s Lease granted on inode %p\n", message,
> > > > >                  &cinode->vfs_inode);
> > > > >  }
> > > > > --
> > > > > 2.1.4
> > > > >
> > > >
> > > >
> > > > --
> > > > Thanks,
> > > >
> > > > Steve
> > > >
> >
> >
> >
> > --
> > Thanks,
> >
> > Steve
> >
Christoph Probst May 7, 2019, 6:10 a.m. UTC | #6 
Steve French schrieb am 06.05.2019 um 23:18 Uhr:

> On Mon, May 6, 2019 at 2:03 PM Pavel Shilovsky
> <pavel.shilovsky@gmail.com> wrote:
> >
> > The patch itself is fine but I think we have a bigger problem here:
> 
> Good point.  Perhaps make update to the same patch to include both changes

I'll update my patch to implement the change suggested by Pavel.

I'll also switch the strcat to strncat and use strncpy in the "None"-case.

Regards,
Christoph
David Laight May 7, 2019, 11:02 a.m. UTC | #7 
From: Christoph Probst
> Sent: 07 May 2019 07:10
> Steve French schrieb am 06.05.2019 um 23:18 Uhr:
> 
> > On Mon, May 6, 2019 at 2:03 PM Pavel Shilovsky
> > <pavel.shilovsky@gmail.com> wrote:
> > >
> > > The patch itself is fine but I think we have a bigger problem here:
> >
> > Good point.  Perhaps make update to the same patch to include both changes
> 
> I'll update my patch to implement the change suggested by Pavel.
> 
> I'll also switch the strcat to strncat and use strncpy in the "None"-case.

strncat() is never the function you are looking for.
The 'n' is the maximum number of bytes to copy, not the length
of the target buffer.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
diff mbox series

Patch

diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
index c36ff0d..5fd5567 100644
--- a/fs/cifs/smb2ops.c
+++ b/fs/cifs/smb2ops.c
@@ -2936,7 +2936,7 @@  smb21_set_oplock_level(struct cifsInodeInfo *cinode, __u32 oplock,
 		strcat(message, "W");
 	}
 	if (!cinode->oplock)
-		strcat(message, "None");
+		strcpy(message, "None");
 	cifs_dbg(FYI, "%s Lease granted on inode %p\n", message,
 		 &cinode->vfs_inode);
 }