* [PATCH cifs segfault ]
@ 2021-04-06 12:17 Seth Thielemann
2021-04-06 14:28 ` Aurélien Aptel
0 siblings, 1 reply; 5+ messages in thread
From: Seth Thielemann @ 2021-04-06 12:17 UTC (permalink / raw)
To: CIFS
From e14cbf048c40d565e31d28479794d1feff9474f0 Mon Sep 17 00:00:00 2001
From: "J. Seth Thielemann" <sthielemann@barracuda.com>
Date: Mon, 5 Apr 2021 14:58:59 -0400
Subject: [PATCH] BNBS-45422: cifs segfault 6.5.03
- Observed segfaults during cifs share backups, core investigation and strace revealed that files were being opened
but upon read the syscall was returning a 32-bit error code:
open("/mnt/datasource/12/USERS8/pjabbia/data/MailPersonalFolders/Personal Folders.pst", O_RDONLY) = 10 <0.001080>
....
read(10, "ZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZ)6 \26
5ZZZZ\3274\325\16\250\177\0\0h%\1\0\0\0\0\0Y\f\0\0\0\0\0\0\27C\225\231ZZZZh\232\202\33\250\177\0\0h\232\202\33\250\177\0\0\320\234\206\33\250\177\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0ZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZ
ZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZ)6 \265ZZZZ\2572\32
5\16\250\177\0\0\300#\1\0\0\0\0\0Y\f\0\0\0\0\0\0\27C\225\231ZZZZ\20\234\202\33\250\177\0\0\20\234\202\33\250\177\0\0\320\234\206\33\250\177\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0ZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZ
ZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZ
ZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZ
ZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZ
ZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZ
ZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZ"..., 8192) = 4294967283 <0.000144>
- Above is an impossible situation, the sign extension was at fault. The two functions using the trinary assignment of rc
in the cifs asio context:
Before:
188d6: bb f0 fe 00 00 mov ebx,0xfef0
188db: 45 85 e4 test r12d,r12d
188de: 44 89 e0 mov eax,r12d <- msl cleared
188e1: 0f 84 6a 01 00 00 je 18a51 <cifs_uncached_writev_complete+0x371>
188e7: 48 8b 7c 24 18 mov rdi,QWORD PTR [rsp+0x18]
188ec: 49 89 85 a8 00 00 00 mov QWORD PTR [r13+0xa8],rax <- saved
After:
189ce: bb f0 fe 00 00 mov ebx,0xfef0
189d3: 48 85 ed test rbp,rbp
189d6: 74 7c je 18a54 <cifs_uncached_writev_complete+0x374>
189d8: 48 8b 7c 24 18 mov rdi,QWORD PTR [rsp+0x18]
189dd: 49 89 ae a8 00 00 00 mov QWORD PTR [r14+0xa8],rbp
- Use ssize_t here to make sure the sign extension is handled properly.
Signed-off-by: J. Seth Thielemann <sthielemann@barracuda.com>
---
fs/cifs/file.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index 31d5787..b2640fc 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -2988,7 +2988,7 @@ static void collect_uncached_write_data(struct cifs_aio_ctx *ctx)
struct cifs_tcon *tcon;
struct cifs_sb_info *cifs_sb;
struct dentry *dentry = ctx->cfile->dentry;
- int rc;
+ ssize_t rc;
tcon = tlink_tcon(ctx->cfile->tlink);
cifs_sb = CIFS_SB(dentry->d_sb);
@@ -3075,7 +3075,7 @@ static ssize_t __cifs_writev(
struct cifs_aio_ctx *ctx;
struct iov_iter saved_from = *from;
size_t len = iov_iter_count(from);
- int rc;
+ ssize_t rc;
/*
* iov_iter_get_pages_alloc doesn't work with ITER_KVEC.
@@ -3689,7 +3689,7 @@ static int cifs_resend_rdata(struct cifs_readdata *rdata,
struct cifs_readdata *rdata, *tmp;
struct iov_iter *to = &ctx->iter;
struct cifs_sb_info *cifs_sb;
- int rc;
+ ssize_t rc;
cifs_sb = CIFS_SB(ctx->cfile->dentry->d_sb);
@@ -3910,7 +3910,7 @@ ssize_t cifs_user_readv(struct kiocb *iocb, struct iov_iter *to)
struct cifsFileInfo *cfile = (struct cifsFileInfo *)
iocb->ki_filp->private_data;
struct cifs_tcon *tcon = tlink_tcon(cfile->tlink);
- int rc = -EACCES;
+ ssize_t rc = -EACCES;
/*
* In strict cache mode we need to read from the server all the time
--
1.8.3.1
===========================================================
Get the 13 Email Threat Types eBook
https://www.barracuda.com/13-threats-report?utm_source=email_signature&utm_campaign=13tt&utm_medium=email&utm_content=13tt-ebook
DISCLAIMER:
This e-mail and any attachments to it contain confidential and proprietary material of Barracuda, its affiliates or agents, and is solely for the use of the intended recipient. Any review, use, disclosure, distribution or copying of this transmittal is prohibited except by or on behalf of the intended recipient. If you have received this transmittal in error, please notify the sender and destroy this e-mail and any attachments and all copies, whether electronic or printed.
===========================================================
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH cifs segfault ]
2021-04-06 12:17 [PATCH cifs segfault ] Seth Thielemann
@ 2021-04-06 14:28 ` Aurélien Aptel
2021-04-06 16:35 ` Seth Thielemann
0 siblings, 1 reply; 5+ messages in thread
From: Aurélien Aptel @ 2021-04-06 14:28 UTC (permalink / raw)
To: Seth Thielemann, CIFS
Hi Seth,
Seth Thielemann <sthielemann@barracuda.com> writes:
> - Observed segfaults during cifs share backups, core investigation and strace revealed that files were being opened
> but upon read the syscall was returning a 32-bit error code:
On my system (x86_64)
- ssize_t is signed and long which is 8 bytes
- int is signed and 4 bytes
That is a weird bug. Casting (long)-13 to int is ok because -13 is
representable in both.
> - Above is an impossible situation, the sign extension was at fault. The two functions using the trinary assignment of rc
> in the cifs asio context:
You mean this line in collect_uncached_write_data() right? I think the
current code changed from your version, it's in a different function now.
ctx->rc = (rc == 0) ? ctx->total_len : rc;
> 188db: 45 85 e4 test r12d,r12d
Ok I'm assuming r12d is the rc var.
we test if rc == 0 (low 32bit of r12)
> 188de: 44 89 e0 mov eax,r12d <- msl cleared
Now we set eax (low 32bits) to rc (what is msl? most significat l...?).
But the high 32bits are unknown
> 188e1: 0f 84 6a 01 00 00 je 18a51 <cifs_uncached_writev_complete+0x371>
if rc was zero, we take the jump
> 188e7: 48 8b 7c 24 18 mov rdi,QWORD PTR [rsp+0x18]
> 188ec: 49 89 85 a8 00 00 00 mov QWORD PTR [r13+0xa8],rax <- saved
Otherwise we store rax (unknown high 32 bits + low 32bit of rc) in the ctx.
So -13 is
0xfffffff3 (int)
but if you copy it in low part of a zero 64bits you end up with (wrong)
0x00000000fffffff3 (long)
which is 4294967283... should have been 0xfffffffffffffff3
I'm no compiler expert but this looks like possible wrong generated
code for the cast :/
Cheers,
--
Aurélien Aptel / SUSE Labs Samba Team
GPG: 1839 CB5F 9F5B FB9B AA97 8C99 03C8 A49B 521B D5D3
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg, DE
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 247165 (AG München)
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH cifs segfault ]
2021-04-06 14:28 ` Aurélien Aptel
@ 2021-04-06 16:35 ` Seth Thielemann
2021-04-07 14:16 ` Aurélien Aptel
0 siblings, 1 reply; 5+ messages in thread
From: Seth Thielemann @ 2021-04-06 16:35 UTC (permalink / raw)
To: Aurélien Aptel, CIFS
Hi Aurélien,
> From: Aurélien Aptel <aaptel@suse.com>
> Sent: Tuesday, April 6, 2021 10:28 AM
> To: Seth Thielemann <sthielemann@barracuda.com>; CIFS <linux-cifs@vger.kernel.org>
> Subject: Re: [PATCH cifs segfault ]
>
> Hi Seth,
>
> Seth Thielemann <sthielemann@barracuda.com> writes:
> > - Observed segfaults during cifs share backups, core investigation and strace revealed that files were being opened
> > but upon read the syscall was returning a 32-bit error code:
>
> On my system (x86_64)
> - ssize_t is signed and long which is 8 bytes
> - int is signed and 4 bytes
>
> That is a weird bug. Casting (long)-13 to int is ok because -13 is
> representable in both.
>
> > - Above is an impossible situation, the sign extension was at fault. The two functions using the trinary assignment of rc
> > in the cifs asio context:
>
> You mean this line in collect_uncached_write_data() right? I think the
> current code changed from your version, it's in a different function now.
>
> ctx->rc = (rc == 0) ? ctx->total_len : rc;
>
Right, the rc is a ssize_t in cifs_aio_ctx, this was found on a slightly older kernel, would like to see it fixed in mainline, just changed the original patch for updated kernel version.
> > 188db: 45 85 e4 test r12d,r12d
>
> Ok I'm assuming r12d is the rc var.
> we test if rc == 0 (low 32bit of r12)
>
> > 188de: 44 89 e0 mov eax,r12d <- msl cleared
>
> Now we set eax (low 32bits) to rc (what is msl? most significat l...?).
> But the high 32bits are unknown
Right, the most significant long / double word gets cleared in the move, such that a few instructions down it gets stored as if it was a 32-bit / int but it's going into a 64-bit variable.
>
> > 188e1: 0f 84 6a 01 00 00 je 18a51 <cifs_uncached_writev_complete+0x371>
>
> if rc was zero, we take the jump
>
> 188e7: 48 8b 7c 24 18 mov rdi,QWORD PTR [rsp+0x18]
> 188ec: 49 89 85 a8 00 00 00 mov QWORD PTR [r13+0xa8],rax <- saved
>
> Otherwise we store rax (unknown high 32 bits + low 32bit of rc) in the ctx.
>
> So -13 is
>
> 0xfffffff3 (int)
>
> but if you copy it in low part of a zero 64bits you end up with (wrong)
>
> 0x00000000fffffff3 (long)
>
> which is 4294967283... should have been 0xfffffffffffffff3
>
> I'm no compiler expert but this looks like possible wrong generated
> code for the cast :/
This definitely could be a bug with the compiler, I ran into issues adding some printk's and things just magically worked and then changed to adding asm volatile nop sentinel's to make sure I was looking at the correct sections. I still think it's a reasonable change to use the ssize_t since the rc is a ssize_t and the outbound syscall path is also a ssize_t. Best case scenario is a segfault in userspace (made things easier to track down), but will likely wind up with memory corruption otherwise.
Thanks,
Seth
Cheers,
--
Aurélien Aptel / SUSE Labs Samba Team
GPG: 1839 CB5F 9F5B FB9B AA97 8C99 03C8 A49B 521B D5D3
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg, DE
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 247165 (AG München)
===========================================================
Get the 13 Email Threat Types eBook
https://www.barracuda.com/13-threats-report?utm_source=email_signature&utm_campaign=13tt&utm_medium=email&utm_content=13tt-ebook
DISCLAIMER:
This e-mail and any attachments to it contain confidential and proprietary material of Barracuda, its affiliates or agents, and is solely for the use of the intended recipient. Any review, use, disclosure, distribution or copying of this transmittal is prohibited except by or on behalf of the intended recipient. If you have received this transmittal in error, please notify the sender and destroy this e-mail and any attachments and all copies, whether electronic or printed.
===========================================================
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH cifs segfault ]
2021-04-06 16:35 ` Seth Thielemann
@ 2021-04-07 14:16 ` Aurélien Aptel
2021-04-09 4:18 ` Steve French
0 siblings, 1 reply; 5+ messages in thread
From: Aurélien Aptel @ 2021-04-07 14:16 UTC (permalink / raw)
To: Seth Thielemann, CIFS
Seth Thielemann <sthielemann@barracuda.com> writes:
> This definitely could be a bug with the compiler, I ran into issues adding some printk's and things just magically worked and then changed to adding asm volatile nop sentinel's to make sure I was looking at the correct sections. I still think it's a reasonable change to use the ssize_t since the rc is a ssize_t and the outbound syscall path is also a ssize_t. Best case scenario is a segfault in userspace (made things easier to track down), but will likely wind up with memory corruption otherwise.
Looking at this more I found that commit 97adda8b3ab7 fixed a very
similar issue:
- ctx->rc = (rc == 0) ? ctx->total_len : rc;
+ ctx->rc = (rc == 0) ? (ssize_t)ctx->total_len : rc;
I think the logic is that compiler sees the "then" part as unsigned and
so casts the "else" part to unsigned as well.
In any case I think the change is good. We could change rc type in the
read path as well.
Reviewed-by: Aurelien Aptel <aaptel@suse.com>
Cheers,
--
Aurélien Aptel / SUSE Labs Samba Team
GPG: 1839 CB5F 9F5B FB9B AA97 8C99 03C8 A49B 521B D5D3
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg, DE
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 247165 (AG München)
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH cifs segfault ]
2021-04-07 14:16 ` Aurélien Aptel
@ 2021-04-09 4:18 ` Steve French
0 siblings, 0 replies; 5+ messages in thread
From: Steve French @ 2021-04-09 4:18 UTC (permalink / raw)
To: Aurélien Aptel; +Cc: Seth Thielemann, CIFS
This patch didn't apply (presumably due to whitespace issues) - can
you resend it (can be sent offline if you prefer) as an attachment?
I also want to run it with checkpatch/sparse etc.
Also let me know if you see any issues with the read path
On Wed, Apr 7, 2021 at 4:03 PM Aurélien Aptel <aaptel@suse.com> wrote:
>
> Seth Thielemann <sthielemann@barracuda.com> writes:
> > This definitely could be a bug with the compiler, I ran into issues adding some printk's and things just magically worked and then changed to adding asm volatile nop sentinel's to make sure I was looking at the correct sections. I still think it's a reasonable change to use the ssize_t since the rc is a ssize_t and the outbound syscall path is also a ssize_t. Best case scenario is a segfault in userspace (made things easier to track down), but will likely wind up with memory corruption otherwise.
>
> Looking at this more I found that commit 97adda8b3ab7 fixed a very
> similar issue:
>
> - ctx->rc = (rc == 0) ? ctx->total_len : rc;
> + ctx->rc = (rc == 0) ? (ssize_t)ctx->total_len : rc;
>
> I think the logic is that compiler sees the "then" part as unsigned and
> so casts the "else" part to unsigned as well.
>
> In any case I think the change is good. We could change rc type in the
> read path as well.
>
> Reviewed-by: Aurelien Aptel <aaptel@suse.com>
>
> Cheers,
> --
> Aurélien Aptel / SUSE Labs Samba Team
> GPG: 1839 CB5F 9F5B FB9B AA97 8C99 03C8 A49B 521B D5D3
> SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg, DE
> GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 247165 (AG München)
>
--
Thanks,
Steve
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-04-09 4:18 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-06 12:17 [PATCH cifs segfault ] Seth Thielemann
2021-04-06 14:28 ` Aurélien Aptel
2021-04-06 16:35 ` Seth Thielemann
2021-04-07 14:16 ` Aurélien Aptel
2021-04-09 4:18 ` Steve French
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).