APPLIED: [SRU B/D][PATCH 0/1] cifs set_oplock buffer overflow in strcat

[Khaled Elmously]

On 2019-07-17 17:13:08 , Guilherme G. Piccoli wrote:
> BugLink: https://bugs.launchpad.net/bugs/1824981
> 
> [Impact]
> 
> * We got reports of a kernel crash in cifs module with the following signature:
> 
> detected buffer overflow in strcat
> kernel BUG at <...>/lib/string.c:1052!
> invalid opcode: 0000 [#1] SMP PTI
> RIP: 0010:fortify_panic+0x13/0x1f
> Call Trace:
>  smb21_set_oplock_level+0xde/0x190 [cifs]
>  smb3_set_oplock_level+0x22/0x90 [cifs]
>  smb2_set_fid+0x76/0xb0 [cifs]
>  cifs_new_fileinfo+0x268/0x3c0 [cifs]
>  ? smb2_get_lease_key+0x40/0x40 [cifs]
>  ? cifs_new_fileinfo+0x268/0x3c0 [cifs]
>  cifs_open+0x57c/0x8d0 [cifs]
>  do_dentry_open+0x1fe/0x320
> [...]
> 
> * By analyzing the code of smb21_set_oplock_level(), we've noticed the only way
> fortify function strcat() would get overflow was if the value of cinode->oplock
> got corrupted in a another thread leading to a buffer write bigger then buffer
> size. In this function, the 'message' buffer writes are governed by cinode->oplock,
> so only a different thread cleaning the oplock value would lead to 'message' overflow.
> 
> * By the same time we worked this analysis, a fix was proposed upstream for this
> issue in the form of commit 6a54b2e002c9 ("cifs: fix strcat buffer overflow and
> reduce raciness in smb21_set_oplock_level()"). The fix is simple and directly
> addresses this problem, so we hereby request its SRU into Bionic kernel - it's
> already present in linux stable branches.
> 
> [Test case]
> 
> * Unfortunately we cannot reproduce the issue. The patch proposed here was
> validated by us with xfstests (instructions followed from
> https://wiki.samba.org/index.php/Xfstesting-cifs) and fio.
> 
> * Using xfstest with the exclusions proposed in the link above we managed to get
> the same results as a non-patched kernel, i.e., the same tests failed in both
> kernels, we didn't get worse results with the patch. Fio also didn't show
> noticeable performance regression with the patch.
> 
> [Regression potential]
> 
> * The patch was validated by the cifs filesystem maintainers and by the
> aforementioned tests; also, the scope is restricted to cifs only so the likelihood
> of regressions is considered low. The commit introduces no functional changes
> and the only affected path was just refactored in a way to prevent overflow
> and reduce race potential.
> 
> 
> Christoph Probst (1):
>   cifs: fix strcat buffer overflow and reduce raciness in
>     smb21_set_oplock_level()
> 
>  fs/cifs/smb2ops.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> -- 
> 2.22.0
> 
> 
> -- 
> kernel-team mailing list
> kernel-team at lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team