Re: [PATCH v2] cifs: fix strcat buffer overflow and reduce raciness in smb21_set_oplock_level()

[Pavel Shilovsky]

ÑÑ, 8 ÐÐÑ 2019 Ð. Ð 01:23, Kai-Heng Feng <kai.heng.feng@xxxxxxxxxxxxx>:
>
> at 02:28, Pavel Shilovsky <piastryyy@xxxxxxxxx> wrote:
>
> > ÐÑ, 7 ÐÐÑ 2019 Ð. Ð 09:13, Steve French via samba-technical
> > <samba-technical@xxxxxxxxxxxxxxx>:
> >> merged into cifs-2.6.git for-next
> >>
> >> On Tue, May 7, 2019 at 10:17 AM Christoph Probst via samba-technical
> >> <samba-technical@xxxxxxxxxxxxxxx> 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@xxxxxxxxx>
> >>> ---
> >>>  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