[SRU,B/D,1/1] cifs: fix strcat buffer overflow and reduce raciness in smb21_set_oplock_level()
diff mbox series

Message ID 20190717201309.15162-2-gpiccoli@canonical.com
State New
Headers show
Series
  • cifs set_oplock buffer overflow in strcat
Related show

Commit Message

Guilherme G. Piccoli July 17, 2019, 8:13 p.m. UTC
From: Christoph Probst <kernel@probst.it>

BugLink: https://bugs.launchpad.net/bugs/1824981

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>
Reviewed-by: Pavel Shilovsky <pshilov@microsoft.com>
Signed-off-by: Steve French <stfrench@microsoft.com>
CC: Stable <stable@vger.kernel.org>
(cherry picked from commit 6a54b2e002c9d00b398d35724c79f9fe0d9b38fb)
Signed-off-by: Guilherme G. Piccoli <gpiccoli@canonical.com>
---
 fs/cifs/smb2ops.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

Comments

Stefan Bader July 18, 2019, 9:24 a.m. UTC | #1
On 17.07.19 22:13, Guilherme G. Piccoli wrote:
> From: Christoph Probst <kernel@probst.it>
> 
> BugLink: https://bugs.launchpad.net/bugs/1824981
> 
> 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>
> Reviewed-by: Pavel Shilovsky <pshilov@microsoft.com>
> Signed-off-by: Steve French <stfrench@microsoft.com>
> CC: Stable <stable@vger.kernel.org>
> (cherry picked from commit 6a54b2e002c9d00b398d35724c79f9fe0d9b38fb)
> Signed-off-by: Guilherme G. Piccoli <gpiccoli@canonical.com>
Acked-by: Stefan Bader <stefan.bader@canonical.com>
> ---
>  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 f5eeecb5cbc3..24835e002941 100644
> --- a/fs/cifs/smb2ops.c
> +++ b/fs/cifs/smb2ops.c
> @@ -1933,26 +1933,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);
>  }
>
Marcelo Henrique Cerri July 18, 2019, 1:25 p.m. UTC | #2
Acked-by: Marcelo Henrique Cerri <marcelo.cerri@canonical.com>

Is that needed for 4.4 too?

On Wed, Jul 17, 2019 at 05:13:09PM -0300, Guilherme G. Piccoli wrote:
> From: Christoph Probst <kernel@probst.it>
> 
> BugLink: https://bugs.launchpad.net/bugs/1824981
> 
> 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>
> Reviewed-by: Pavel Shilovsky <pshilov@microsoft.com>
> Signed-off-by: Steve French <stfrench@microsoft.com>
> CC: Stable <stable@vger.kernel.org>
> (cherry picked from commit 6a54b2e002c9d00b398d35724c79f9fe0d9b38fb)
> Signed-off-by: Guilherme G. Piccoli <gpiccoli@canonical.com>
> ---
>  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 f5eeecb5cbc3..24835e002941 100644
> --- a/fs/cifs/smb2ops.c
> +++ b/fs/cifs/smb2ops.c
> @@ -1933,26 +1933,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.22.0
> 
> 
> -- 
> kernel-team mailing list
> kernel-team@lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team
Guilherme G. Piccoli July 18, 2019, 1:48 p.m. UTC | #3
On Thu, Jul 18, 2019 at 10:25 AM Marcelo Henrique Cerri
<marcelo.cerri@canonical.com> wrote:
>
> Acked-by: Marcelo Henrique Cerri <marcelo.cerri@canonical.com>
>
> Is that needed for 4.4 too?

Hi Cerri, thanks for the ack! It's on stable tree, v4.4.185 - it'll
arrive organically in Ubuntu
and since kernel 4.4 has no fortify panic and most cloud providers
users (if not all) are using
4.15, I don't see a need for SRU to Xenial now (specially given the
big size of this cycle).
If you or anybody else disagree with this, I'll be glad to provide SRU
for Xenial.

Cheers,


Guilherme

Patch
diff mbox series

diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
index f5eeecb5cbc3..24835e002941 100644
--- a/fs/cifs/smb2ops.c
+++ b/fs/cifs/smb2ops.c
@@ -1933,26 +1933,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);
 }