Messages in this thread | | | Subject | Re: [PATCH v2] cifs: fix strcat buffer overflow and reduce raciness in smb21_set_oplock_level() | From | Kai-Heng Feng <> | Date | Thu, 9 May 2019 03:08:19 +0800 |
| |
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
| |