| Message ID | DM6PR10MB3833F0DD867BF1B48F60B99FA2769@DM6PR10MB3833.namprd10.prod.outlook.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [cifs,segfault] | expand |
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,
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,
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,
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) >
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