Messages in this thread Patch in this message | | | Subject | Re: [PATCH v2] cifs: fix strcat buffer overflow and reduce raciness in smb21_set_oplock_level() | From | Kai-Heng Feng <> | Date | Wed, 8 May 2019 16:23:42 +0800 |
| |
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
| |