* [PATCH v2] cifs: fix strcat buffer overflow and reduce raciness in smb21_set_oplock_level()
@ 2019-05-07 15:16 Christoph Probst
2019-05-07 16:13 ` Steve French
0 siblings, 1 reply; 6+ messages in thread
From: Christoph Probst @ 2019-05-07 15:16 UTC (permalink / raw)
To: linux-cifs; +Cc: Steve French, samba-technical, linux-kernel, Christoph Probst
Change strcat to strncpy in the "None" case to fix a buffer overflow
when cinode->oplock is reset to 0 by another thread accessing the same
cinode. It is never valid to append "None" to any other message.
Consolidate multiple writes to cinode->oplock to reduce raciness.
Signed-off-by: Christoph Probst <kernel@probst.it>
---
fs/cifs/smb2ops.c | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
index c36ff0d..aa61dcf 100644
--- a/fs/cifs/smb2ops.c
+++ b/fs/cifs/smb2ops.c
@@ -2917,26 +2917,28 @@ smb21_set_oplock_level(struct cifsInodeInfo *cinode, __u32 oplock,
unsigned int epoch, bool *purge_cache)
{
char message[5] = {0};
+ unsigned int new_oplock = 0;
oplock &= 0xFF;
if (oplock == SMB2_OPLOCK_LEVEL_NOCHANGE)
return;
- cinode->oplock = 0;
if (oplock & SMB2_LEASE_READ_CACHING_HE) {
- cinode->oplock |= CIFS_CACHE_READ_FLG;
+ new_oplock |= CIFS_CACHE_READ_FLG;
strcat(message, "R");
}
if (oplock & SMB2_LEASE_HANDLE_CACHING_HE) {
- cinode->oplock |= CIFS_CACHE_HANDLE_FLG;
+ new_oplock |= CIFS_CACHE_HANDLE_FLG;
strcat(message, "H");
}
if (oplock & SMB2_LEASE_WRITE_CACHING_HE) {
- cinode->oplock |= CIFS_CACHE_WRITE_FLG;
+ new_oplock |= CIFS_CACHE_WRITE_FLG;
strcat(message, "W");
}
- if (!cinode->oplock)
- strcat(message, "None");
+ if (!new_oplock)
+ strncpy(message, "None", sizeof(message));
+
+ cinode->oplock = new_oplock;
cifs_dbg(FYI, "%s Lease granted on inode %p\n", message,
&cinode->vfs_inode);
}
--
2.1.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] cifs: fix strcat buffer overflow and reduce raciness in smb21_set_oplock_level()
2019-05-07 15:16 [PATCH v2] cifs: fix strcat buffer overflow and reduce raciness in smb21_set_oplock_level() Christoph Probst
@ 2019-05-07 16:13 ` Steve French
2019-05-07 18:28 ` Pavel Shilovsky
0 siblings, 1 reply; 6+ messages in thread
From: Steve French @ 2019-05-07 16:13 UTC (permalink / raw)
To: Christoph Probst; +Cc: CIFS, Steve French, samba-technical, LKML
merged into cifs-2.6.git for-next
On Tue, May 7, 2019 at 10:17 AM Christoph Probst via samba-technical
<samba-technical@lists.samba.org> wrote:
>
> Change strcat to strncpy in the "None" case to fix a buffer overflow
> when cinode->oplock is reset to 0 by another thread accessing the same
> cinode. It is never valid to append "None" to any other message.
>
> Consolidate multiple writes to cinode->oplock to reduce raciness.
>
> Signed-off-by: Christoph Probst <kernel@probst.it>
> ---
> fs/cifs/smb2ops.c | 14 ++++++++------
> 1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
> index c36ff0d..aa61dcf 100644
> --- a/fs/cifs/smb2ops.c
> +++ b/fs/cifs/smb2ops.c
> @@ -2917,26 +2917,28 @@ smb21_set_oplock_level(struct cifsInodeInfo *cinode, __u32 oplock,
> unsigned int epoch, bool *purge_cache)
> {
> char message[5] = {0};
> + unsigned int new_oplock = 0;
>
> oplock &= 0xFF;
> if (oplock == SMB2_OPLOCK_LEVEL_NOCHANGE)
> return;
>
> - cinode->oplock = 0;
> if (oplock & SMB2_LEASE_READ_CACHING_HE) {
> - cinode->oplock |= CIFS_CACHE_READ_FLG;
> + new_oplock |= CIFS_CACHE_READ_FLG;
> strcat(message, "R");
> }
> if (oplock & SMB2_LEASE_HANDLE_CACHING_HE) {
> - cinode->oplock |= CIFS_CACHE_HANDLE_FLG;
> + new_oplock |= CIFS_CACHE_HANDLE_FLG;
> strcat(message, "H");
> }
> if (oplock & SMB2_LEASE_WRITE_CACHING_HE) {
> - cinode->oplock |= CIFS_CACHE_WRITE_FLG;
> + new_oplock |= CIFS_CACHE_WRITE_FLG;
> strcat(message, "W");
> }
> - if (!cinode->oplock)
> - strcat(message, "None");
> + if (!new_oplock)
> + strncpy(message, "None", sizeof(message));
> +
> + cinode->oplock = new_oplock;
> cifs_dbg(FYI, "%s Lease granted on inode %p\n", message,
> &cinode->vfs_inode);
> }
> --
> 2.1.4
>
>
--
Thanks,
Steve
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] cifs: fix strcat buffer overflow and reduce raciness in smb21_set_oplock_level()
2019-05-07 16:13 ` Steve French
@ 2019-05-07 18:28 ` Pavel Shilovsky
2019-05-08 8:23 ` Kai-Heng Feng
0 siblings, 1 reply; 6+ messages in thread
From: Pavel Shilovsky @ 2019-05-07 18:28 UTC (permalink / raw)
To: Steve French; +Cc: Christoph Probst, Steve French, CIFS, samba-technical, LKML
вт, 7 мая 2019 г. в 09:13, Steve French via samba-technical
<samba-technical@lists.samba.org>:
>
> merged into cifs-2.6.git for-next
>
> On Tue, May 7, 2019 at 10:17 AM Christoph Probst via samba-technical
> <samba-technical@lists.samba.org> wrote:
> >
> > Change strcat to strncpy in the "None" case to fix a buffer overflow
> > when cinode->oplock is reset to 0 by another thread accessing the same
> > cinode. It is never valid to append "None" to any other message.
> >
> > Consolidate multiple writes to cinode->oplock to reduce raciness.
> >
> > Signed-off-by: Christoph Probst <kernel@probst.it>
> > ---
> > fs/cifs/smb2ops.c | 14 ++++++++------
> > 1 file changed, 8 insertions(+), 6 deletions(-)
> >
> > diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
> > index c36ff0d..aa61dcf 100644
> > --- a/fs/cifs/smb2ops.c
> > +++ b/fs/cifs/smb2ops.c
> > @@ -2917,26 +2917,28 @@ smb21_set_oplock_level(struct cifsInodeInfo *cinode, __u32 oplock,
> > unsigned int epoch, bool *purge_cache)
> > {
> > char message[5] = {0};
> > + unsigned int new_oplock = 0;
> >
> > oplock &= 0xFF;
> > if (oplock == SMB2_OPLOCK_LEVEL_NOCHANGE)
> > return;
> >
> > - cinode->oplock = 0;
> > if (oplock & SMB2_LEASE_READ_CACHING_HE) {
> > - cinode->oplock |= CIFS_CACHE_READ_FLG;
> > + new_oplock |= CIFS_CACHE_READ_FLG;
> > strcat(message, "R");
> > }
> > if (oplock & SMB2_LEASE_HANDLE_CACHING_HE) {
> > - cinode->oplock |= CIFS_CACHE_HANDLE_FLG;
> > + new_oplock |= CIFS_CACHE_HANDLE_FLG;
> > strcat(message, "H");
> > }
> > if (oplock & SMB2_LEASE_WRITE_CACHING_HE) {
> > - cinode->oplock |= CIFS_CACHE_WRITE_FLG;
> > + new_oplock |= CIFS_CACHE_WRITE_FLG;
> > strcat(message, "W");
> > }
> > - if (!cinode->oplock)
> > - strcat(message, "None");
> > + if (!new_oplock)
> > + strncpy(message, "None", sizeof(message));
> > +
> > + cinode->oplock = new_oplock;
> > cifs_dbg(FYI, "%s Lease granted on inode %p\n", message,
> > &cinode->vfs_inode);
> > }
> > --
> > 2.1.4
> >
> >
>
Thanks for cleaning it up!
Reviewed-by: Pavel Shilovsky <pshilov@microsoft.com>
--
Best regards,
Pavel Shilovsky
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] cifs: fix strcat buffer overflow and reduce raciness in smb21_set_oplock_level()
2019-05-07 18:28 ` Pavel Shilovsky
@ 2019-05-08 8:23 ` Kai-Heng Feng
2019-05-08 18:42 ` Pavel Shilovsky
0 siblings, 1 reply; 6+ messages in thread
From: Kai-Heng Feng @ 2019-05-08 8:23 UTC (permalink / raw)
To: Pavel Shilovsky
Cc: Steve French, Christoph Probst, Steve French, CIFS,
samba-technical, LKML
at 02:28, Pavel Shilovsky <piastryyy@gmail.com> wrote:
> вт, 7 мая 2019 г. в 09:13, Steve French via samba-technical
> <samba-technical@lists.samba.org>:
>> merged into cifs-2.6.git for-next
>>
>> On Tue, May 7, 2019 at 10:17 AM Christoph Probst via samba-technical
>> <samba-technical@lists.samba.org> wrote:
>>> Change strcat to strncpy in the "None" case to fix a buffer overflow
>>> when cinode->oplock is reset to 0 by another thread accessing the same
>>> cinode. It is never valid to append "None" to any other message.
>>>
>>> Consolidate multiple writes to cinode->oplock to reduce raciness.
>>>
>>> Signed-off-by: Christoph Probst <kernel@probst.it>
>>> ---
>>> fs/cifs/smb2ops.c | 14 ++++++++------
>>> 1 file changed, 8 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
>>> index c36ff0d..aa61dcf 100644
>>> --- a/fs/cifs/smb2ops.c
>>> +++ b/fs/cifs/smb2ops.c
>>> @@ -2917,26 +2917,28 @@ smb21_set_oplock_level(struct cifsInodeInfo
>>> *cinode, __u32 oplock,
>>> unsigned int epoch, bool *purge_cache)
>>> {
>>> char message[5] = {0};
>>> + unsigned int new_oplock = 0;
>>>
>>> oplock &= 0xFF;
>>> if (oplock == SMB2_OPLOCK_LEVEL_NOCHANGE)
>>> return;
>>>
>>> - cinode->oplock = 0;
>>> if (oplock & SMB2_LEASE_READ_CACHING_HE) {
>>> - cinode->oplock |= CIFS_CACHE_READ_FLG;
>>> + new_oplock |= CIFS_CACHE_READ_FLG;
>>> strcat(message, "R");
>>> }
>>> if (oplock & SMB2_LEASE_HANDLE_CACHING_HE) {
>>> - cinode->oplock |= CIFS_CACHE_HANDLE_FLG;
>>> + new_oplock |= CIFS_CACHE_HANDLE_FLG;
>>> strcat(message, "H");
>>> }
>>> if (oplock & SMB2_LEASE_WRITE_CACHING_HE) {
>>> - cinode->oplock |= CIFS_CACHE_WRITE_FLG;
>>> + new_oplock |= CIFS_CACHE_WRITE_FLG;
>>> strcat(message, "W");
>>> }
>>> - if (!cinode->oplock)
>>> - strcat(message, "None");
>>> + if (!new_oplock)
>>> + strncpy(message, "None", sizeof(message));
>>> +
>>> + cinode->oplock = new_oplock;
>>> cifs_dbg(FYI, "%s Lease granted on inode %p\n", message,
>>> &cinode->vfs_inode);
>>> }
>>> --
>>> 2.1.4
>
Doesn’t the race still happen, but implicitly here?
cinode->oplock = new_oplock;
Is it possible to just introduce a lock to force its proper ordering?
e.g.
diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index bf5b8264e119..a3c3c6156d17 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -267,6 +267,7 @@ cifs_alloc_inode(struct super_block *sb)
* server, can not assume caching of file data or metadata.
*/
cifs_set_oplock_level(cifs_inode, 0);
+ mutex_init(&cifs_inode->oplock_mutex);
cifs_inode->flags = 0;
spin_lock_init(&cifs_inode->writers_lock);
cifs_inode->writers = 0;
diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index 37b5ddf27ff1..6dfd4ab16c4f 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -1214,6 +1214,7 @@ struct cifsInodeInfo {
struct list_head openFileList;
__u32 cifsAttrs; /* e.g. DOS archive bit, sparse, compressed, system */
unsigned int oplock; /* oplock/lease level we have */
+ struct mutex oplock_mutex;
unsigned int epoch; /* used to track lease state changes */
#define CIFS_INODE_PENDING_OPLOCK_BREAK (0) /* oplock break in progress */
#define CIFS_INODE_PENDING_WRITERS (1) /* Writes in progress */
diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
index b20063cf774f..796b23712e71 100644
--- a/fs/cifs/smb2ops.c
+++ b/fs/cifs/smb2ops.c
@@ -1901,6 +1901,7 @@ smb21_set_oplock_level(struct cifsInodeInfo *cinode,
__u32 oplock,
if (oplock == SMB2_OPLOCK_LEVEL_NOCHANGE)
return;
+ mutex_lock(&cinode->oplock_mutex);
cinode->oplock = 0;
if (oplock & SMB2_LEASE_READ_CACHING_HE) {
cinode->oplock |= CIFS_CACHE_READ_FLG;
@@ -1916,6 +1917,8 @@ smb21_set_oplock_level(struct cifsInodeInfo *cinode,
__u32 oplock,
}
if (!cinode->oplock)
strcat(message, "None");
+ mutex_unlock(&cinode->oplock_mutex);
+
cifs_dbg(FYI, "%s Lease granted on inode %p\n", message,
&cinode->vfs_inode);
}
Kai-Heng
> Thanks for cleaning it up!
>
> Reviewed-by: Pavel Shilovsky <pshilov@microsoft.com>
>
> --
> Best regards,
> Pavel Shilovsky
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] cifs: fix strcat buffer overflow and reduce raciness in smb21_set_oplock_level()
2019-05-08 8:23 ` Kai-Heng Feng
@ 2019-05-08 18:42 ` Pavel Shilovsky
2019-05-08 19:08 ` Kai-Heng Feng
0 siblings, 1 reply; 6+ messages in thread
From: Pavel Shilovsky @ 2019-05-08 18:42 UTC (permalink / raw)
To: Kai-Heng Feng
Cc: Steve French, Christoph Probst, Steve French, CIFS,
samba-technical, LKML
ср, 8 мая 2019 г. в 01:23, Kai-Heng Feng <kai.heng.feng@canonical.com>:
>
> at 02:28, Pavel Shilovsky <piastryyy@gmail.com> wrote:
>
> > вт, 7 мая 2019 г. в 09:13, Steve French via samba-technical
> > <samba-technical@lists.samba.org>:
> >> merged into cifs-2.6.git for-next
> >>
> >> On Tue, May 7, 2019 at 10:17 AM Christoph Probst via samba-technical
> >> <samba-technical@lists.samba.org> wrote:
> >>> Change strcat to strncpy in the "None" case to fix a buffer overflow
> >>> when cinode->oplock is reset to 0 by another thread accessing the same
> >>> cinode. It is never valid to append "None" to any other message.
> >>>
> >>> Consolidate multiple writes to cinode->oplock to reduce raciness.
> >>>
> >>> Signed-off-by: Christoph Probst <kernel@probst.it>
> >>> ---
> >>> fs/cifs/smb2ops.c | 14 ++++++++------
> >>> 1 file changed, 8 insertions(+), 6 deletions(-)
> >>>
> >>> diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
> >>> index c36ff0d..aa61dcf 100644
> >>> --- a/fs/cifs/smb2ops.c
> >>> +++ b/fs/cifs/smb2ops.c
> >>> @@ -2917,26 +2917,28 @@ smb21_set_oplock_level(struct cifsInodeInfo
> >>> *cinode, __u32 oplock,
> >>> unsigned int epoch, bool *purge_cache)
> >>> {
> >>> char message[5] = {0};
> >>> + unsigned int new_oplock = 0;
> >>>
> >>> oplock &= 0xFF;
> >>> if (oplock == SMB2_OPLOCK_LEVEL_NOCHANGE)
> >>> return;
> >>>
> >>> - cinode->oplock = 0;
> >>> if (oplock & SMB2_LEASE_READ_CACHING_HE) {
> >>> - cinode->oplock |= CIFS_CACHE_READ_FLG;
> >>> + new_oplock |= CIFS_CACHE_READ_FLG;
> >>> strcat(message, "R");
> >>> }
> >>> if (oplock & SMB2_LEASE_HANDLE_CACHING_HE) {
> >>> - cinode->oplock |= CIFS_CACHE_HANDLE_FLG;
> >>> + new_oplock |= CIFS_CACHE_HANDLE_FLG;
> >>> strcat(message, "H");
> >>> }
> >>> if (oplock & SMB2_LEASE_WRITE_CACHING_HE) {
> >>> - cinode->oplock |= CIFS_CACHE_WRITE_FLG;
> >>> + new_oplock |= CIFS_CACHE_WRITE_FLG;
> >>> strcat(message, "W");
> >>> }
> >>> - if (!cinode->oplock)
> >>> - strcat(message, "None");
> >>> + if (!new_oplock)
> >>> + strncpy(message, "None", sizeof(message));
> >>> +
> >>> + cinode->oplock = new_oplock;
> >>> cifs_dbg(FYI, "%s Lease granted on inode %p\n", message,
> >>> &cinode->vfs_inode);
> >>> }
> >>> --
> >>> 2.1.4
> >
>
> Doesn’t the race still happen, but implicitly here?
> cinode->oplock = new_oplock;
>
> Is it possible to just introduce a lock to force its proper ordering?
> e.g.
>
> diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> index bf5b8264e119..a3c3c6156d17 100644
> --- a/fs/cifs/cifsfs.c
> +++ b/fs/cifs/cifsfs.c
> @@ -267,6 +267,7 @@ cifs_alloc_inode(struct super_block *sb)
> * server, can not assume caching of file data or metadata.
> */
> cifs_set_oplock_level(cifs_inode, 0);
> + mutex_init(&cifs_inode->oplock_mutex);
> cifs_inode->flags = 0;
> spin_lock_init(&cifs_inode->writers_lock);
> cifs_inode->writers = 0;
> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> index 37b5ddf27ff1..6dfd4ab16c4f 100644
> --- a/fs/cifs/cifsglob.h
> +++ b/fs/cifs/cifsglob.h
> @@ -1214,6 +1214,7 @@ struct cifsInodeInfo {
> struct list_head openFileList;
> __u32 cifsAttrs; /* e.g. DOS archive bit, sparse, compressed, system */
> unsigned int oplock; /* oplock/lease level we have */
> + struct mutex oplock_mutex;
> unsigned int epoch; /* used to track lease state changes */
> #define CIFS_INODE_PENDING_OPLOCK_BREAK (0) /* oplock break in progress */
> #define CIFS_INODE_PENDING_WRITERS (1) /* Writes in progress */
> diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
> index b20063cf774f..796b23712e71 100644
> --- a/fs/cifs/smb2ops.c
> +++ b/fs/cifs/smb2ops.c
> @@ -1901,6 +1901,7 @@ smb21_set_oplock_level(struct cifsInodeInfo *cinode,
> __u32 oplock,
> if (oplock == SMB2_OPLOCK_LEVEL_NOCHANGE)
> return;
>
> + mutex_lock(&cinode->oplock_mutex);
> cinode->oplock = 0;
> if (oplock & SMB2_LEASE_READ_CACHING_HE) {
> cinode->oplock |= CIFS_CACHE_READ_FLG;
> @@ -1916,6 +1917,8 @@ smb21_set_oplock_level(struct cifsInodeInfo *cinode,
> __u32 oplock,
> }
> if (!cinode->oplock)
> strcat(message, "None");
> + mutex_unlock(&cinode->oplock_mutex);
> +
> cifs_dbg(FYI, "%s Lease granted on inode %p\n", message,
> &cinode->vfs_inode);
> }
>
> Kai-Heng
Unless you calculations on the oplock value or accessing it multiple
times with some logic involved I don't think locking will help much.
If two threads are assigning the same variable, you can end up with
two possible outcomes regardless of whether locking is used or not.
Locking will be needed once we start to make proper decisions based on
previous and new values of the oplock to purge a page cache or flush
buffered data. This still needs to be done and is out of the scope of
this patch which aims to fix the buffer overflow error.
--
Best regards,
Pavel Shilovsky
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] cifs: fix strcat buffer overflow and reduce raciness in smb21_set_oplock_level()
2019-05-08 18:42 ` Pavel Shilovsky
@ 2019-05-08 19:08 ` Kai-Heng Feng
0 siblings, 0 replies; 6+ messages in thread
From: Kai-Heng Feng @ 2019-05-08 19:08 UTC (permalink / raw)
To: Pavel Shilovsky
Cc: Steve French, Christoph Probst, Steve French, CIFS,
samba-technical, LKML
at 02:42, Pavel Shilovsky <piastryyy@gmail.com> wrote:
> ср, 8 мая 2019 г. в 01:23, Kai-Heng Feng <kai.heng.feng@canonical.com>:
>> at 02:28, Pavel Shilovsky <piastryyy@gmail.com> wrote:
>>
>>> вт, 7 мая 2019 г. в 09:13, Steve French via samba-technical
>>> <samba-technical@lists.samba.org>:
>>>> merged into cifs-2.6.git for-next
>>>>
>>>> On Tue, May 7, 2019 at 10:17 AM Christoph Probst via samba-technical
>>>> <samba-technical@lists.samba.org> wrote:
>>>>> Change strcat to strncpy in the "None" case to fix a buffer overflow
>>>>> when cinode->oplock is reset to 0 by another thread accessing the same
>>>>> cinode. It is never valid to append "None" to any other message.
>>>>>
>>>>> Consolidate multiple writes to cinode->oplock to reduce raciness.
>>>>>
>>>>> Signed-off-by: Christoph Probst <kernel@probst.it>
>>>>> ---
>>>>> fs/cifs/smb2ops.c | 14 ++++++++------
>>>>> 1 file changed, 8 insertions(+), 6 deletions(-)
>>>>>
>>>>> diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
>>>>> index c36ff0d..aa61dcf 100644
>>>>> --- a/fs/cifs/smb2ops.c
>>>>> +++ b/fs/cifs/smb2ops.c
>>>>> @@ -2917,26 +2917,28 @@ smb21_set_oplock_level(struct cifsInodeInfo
>>>>> *cinode, __u32 oplock,
>>>>> unsigned int epoch, bool *purge_cache)
>>>>> {
>>>>> char message[5] = {0};
>>>>> + unsigned int new_oplock = 0;
>>>>>
>>>>> oplock &= 0xFF;
>>>>> if (oplock == SMB2_OPLOCK_LEVEL_NOCHANGE)
>>>>> return;
>>>>>
>>>>> - cinode->oplock = 0;
>>>>> if (oplock & SMB2_LEASE_READ_CACHING_HE) {
>>>>> - cinode->oplock |= CIFS_CACHE_READ_FLG;
>>>>> + new_oplock |= CIFS_CACHE_READ_FLG;
>>>>> strcat(message, "R");
>>>>> }
>>>>> if (oplock & SMB2_LEASE_HANDLE_CACHING_HE) {
>>>>> - cinode->oplock |= CIFS_CACHE_HANDLE_FLG;
>>>>> + new_oplock |= CIFS_CACHE_HANDLE_FLG;
>>>>> strcat(message, "H");
>>>>> }
>>>>> if (oplock & SMB2_LEASE_WRITE_CACHING_HE) {
>>>>> - cinode->oplock |= CIFS_CACHE_WRITE_FLG;
>>>>> + new_oplock |= CIFS_CACHE_WRITE_FLG;
>>>>> strcat(message, "W");
>>>>> }
>>>>> - if (!cinode->oplock)
>>>>> - strcat(message, "None");
>>>>> + if (!new_oplock)
>>>>> + strncpy(message, "None", sizeof(message));
>>>>> +
>>>>> + cinode->oplock = new_oplock;
>>>>> cifs_dbg(FYI, "%s Lease granted on inode %p\n", message,
>>>>> &cinode->vfs_inode);
>>>>> }
>>>>> --
>>>>> 2.1.4
>>
>> Doesn’t the race still happen, but implicitly here?
>> cinode->oplock = new_oplock;
>>
>> Is it possible to just introduce a lock to force its proper ordering?
>> e.g.
>>
>> diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
>> index bf5b8264e119..a3c3c6156d17 100644
>> --- a/fs/cifs/cifsfs.c
>> +++ b/fs/cifs/cifsfs.c
>> @@ -267,6 +267,7 @@ cifs_alloc_inode(struct super_block *sb)
>> * server, can not assume caching of file data or metadata.
>> */
>> cifs_set_oplock_level(cifs_inode, 0);
>> + mutex_init(&cifs_inode->oplock_mutex);
>> cifs_inode->flags = 0;
>> spin_lock_init(&cifs_inode->writers_lock);
>> cifs_inode->writers = 0;
>> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
>> index 37b5ddf27ff1..6dfd4ab16c4f 100644
>> --- a/fs/cifs/cifsglob.h
>> +++ b/fs/cifs/cifsglob.h
>> @@ -1214,6 +1214,7 @@ struct cifsInodeInfo {
>> struct list_head openFileList;
>> __u32 cifsAttrs; /* e.g. DOS archive bit, sparse, compressed, system */
>> unsigned int oplock; /* oplock/lease level we have */
>> + struct mutex oplock_mutex;
>> unsigned int epoch; /* used to track lease state changes */
>> #define CIFS_INODE_PENDING_OPLOCK_BREAK (0) /* oplock break in progress */
>> #define CIFS_INODE_PENDING_WRITERS (1) /* Writes in progress */
>> diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
>> index b20063cf774f..796b23712e71 100644
>> --- a/fs/cifs/smb2ops.c
>> +++ b/fs/cifs/smb2ops.c
>> @@ -1901,6 +1901,7 @@ smb21_set_oplock_level(struct cifsInodeInfo *cinode,
>> __u32 oplock,
>> if (oplock == SMB2_OPLOCK_LEVEL_NOCHANGE)
>> return;
>>
>> + mutex_lock(&cinode->oplock_mutex);
>> cinode->oplock = 0;
>> if (oplock & SMB2_LEASE_READ_CACHING_HE) {
>> cinode->oplock |= CIFS_CACHE_READ_FLG;
>> @@ -1916,6 +1917,8 @@ smb21_set_oplock_level(struct cifsInodeInfo *cinode,
>> __u32 oplock,
>> }
>> if (!cinode->oplock)
>> strcat(message, "None");
>> + mutex_unlock(&cinode->oplock_mutex);
>> +
>> cifs_dbg(FYI, "%s Lease granted on inode %p\n", message,
>> &cinode->vfs_inode);
>> }
>>
>> Kai-Heng
>
> Unless you calculations on the oplock value or accessing it multiple
> times with some logic involved I don't think locking will help much.
> If two threads are assigning the same variable, you can end up with
> two possible outcomes regardless of whether locking is used or not.
Yes you are right, didn’t think of this case.
>
> Locking will be needed once we start to make proper decisions based on
> previous and new values of the oplock to purge a page cache or flush
> buffered data. This still needs to be done and is out of the scope of
> this patch which aims to fix the buffer overflow error.
Thanks for your explanation.
Kai-Heng
>
> --
> Best regards,
> Pavel Shilovsky
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-05-08 19:08 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-07 15:16 [PATCH v2] cifs: fix strcat buffer overflow and reduce raciness in smb21_set_oplock_level() Christoph Probst
2019-05-07 16:13 ` Steve French
2019-05-07 18:28 ` Pavel Shilovsky
2019-05-08 8:23 ` Kai-Heng Feng
2019-05-08 18:42 ` Pavel Shilovsky
2019-05-08 19:08 ` Kai-Heng Feng
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).