* [PATCH] cifs: fix strcat buffer overflow in smb21_set_oplock_level() @ 2019-05-06 15:16 Christoph Probst 2019-05-06 16:53 ` Steve French 0 siblings, 1 reply; 8+ messages in thread From: Christoph Probst @ 2019-05-06 15:16 UTC (permalink / raw) To: linux-cifs; +Cc: Steve French, samba-technical, linux-kernel, Christoph Probst 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] cifs: fix strcat buffer overflow in smb21_set_oplock_level() 2019-05-06 15:16 [PATCH] cifs: fix strcat buffer overflow in smb21_set_oplock_level() Christoph Probst @ 2019-05-06 16:53 ` Steve French 2019-05-06 16:56 ` Jeremy Allison 0 siblings, 1 reply; 8+ messages in thread From: Steve French @ 2019-05-06 16:53 UTC (permalink / raw) To: Christoph Probst; +Cc: CIFS, Steve French, samba-technical, LKML 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 > -- Thanks, Steve ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] cifs: fix strcat buffer overflow in smb21_set_oplock_level() 2019-05-06 16:53 ` Steve French @ 2019-05-06 16:56 ` Jeremy Allison 2019-05-06 17:02 ` Steve French 0 siblings, 1 reply; 8+ messages in thread From: Jeremy Allison @ 2019-05-06 16:56 UTC (permalink / raw) To: Steve French; +Cc: Christoph Probst, Steve French, CIFS, samba-technical, LKML 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 > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] cifs: fix strcat buffer overflow in smb21_set_oplock_level() 2019-05-06 16:56 ` Jeremy Allison @ 2019-05-06 17:02 ` Steve French 2019-05-06 19:03 ` Pavel Shilovsky 0 siblings, 1 reply; 8+ messages in thread From: Steve French @ 2019-05-06 17:02 UTC (permalink / raw) To: Jeremy Allison Cc: Christoph Probst, Steve French, CIFS, samba-technical, LKML 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] cifs: fix strcat buffer overflow in smb21_set_oplock_level() 2019-05-06 17:02 ` Steve French @ 2019-05-06 19:03 ` Pavel Shilovsky 2019-05-06 21:18 ` Steve French 0 siblings, 1 reply; 8+ messages in thread From: Pavel Shilovsky @ 2019-05-06 19:03 UTC (permalink / raw) To: Steve French Cc: Jeremy Allison, Steve French, CIFS, samba-technical, LKML, Christoph Probst 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 > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] cifs: fix strcat buffer overflow in smb21_set_oplock_level() 2019-05-06 19:03 ` Pavel Shilovsky @ 2019-05-06 21:18 ` Steve French 2019-05-07 6:10 ` Christoph Probst 0 siblings, 1 reply; 8+ messages in thread From: Steve French @ 2019-05-06 21:18 UTC (permalink / raw) To: Pavel Shilovsky Cc: Jeremy Allison, Steve French, CIFS, samba-technical, LKML, Christoph Probst 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 > > -- Thanks, Steve ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] cifs: fix strcat buffer overflow in smb21_set_oplock_level() 2019-05-06 21:18 ` Steve French @ 2019-05-07 6:10 ` Christoph Probst 2019-05-07 11:02 ` David Laight 0 siblings, 1 reply; 8+ messages in thread From: Christoph Probst @ 2019-05-07 6:10 UTC (permalink / raw) To: Steve French Cc: Pavel Shilovsky, Jeremy Allison, Steve French, CIFS, samba-technical, LKML 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [PATCH] cifs: fix strcat buffer overflow in smb21_set_oplock_level() 2019-05-07 6:10 ` Christoph Probst @ 2019-05-07 11:02 ` David Laight 0 siblings, 0 replies; 8+ messages in thread From: David Laight @ 2019-05-07 11:02 UTC (permalink / raw) To: 'Christoph Probst', Steve French Cc: Pavel Shilovsky, Jeremy Allison, Steve French, CIFS, samba-technical, LKML 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) ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2019-05-07 11:02 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-05-06 15:16 [PATCH] cifs: fix strcat buffer overflow in smb21_set_oplock_level() Christoph Probst 2019-05-06 16:53 ` Steve French 2019-05-06 16:56 ` Jeremy Allison 2019-05-06 17:02 ` Steve French 2019-05-06 19:03 ` Pavel Shilovsky 2019-05-06 21:18 ` Steve French 2019-05-07 6:10 ` Christoph Probst 2019-05-07 11:02 ` David Laight
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).