Hi Aurélien, > From: Aurélien Aptel <aaptel@xxxxxxxx> > Sent: Tuesday, April 6, 2021 10:28 AM > To: Seth Thielemann <sthielemann@xxxxxxxxxxxxx>; CIFS <linux-cifs@xxxxxxxxxxxxxxx> > Subject: Re: [PATCH cifs segfault ] > > Hi Seth, > > Seth Thielemann <sthielemann@xxxxxxxxxxxxx> 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. ===========================================================
