| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAH2r5mtBb6jZ5rAy2KY9H=aaB6J+=Ti1GyLoNoiWeLGSGNxNVg@mail.gmail.com>
Date: Wed, 16 Jun 2021 12:09:54 -0500
From: Steve French <smfrench@...il.com>
To: ronnie sahlberg <ronniesahlberg@...il.com>
Cc: Thiago Rafael Becker <trbecker@...il.com>,
linux-cifs <linux-cifs@...r.kernel.org>,
Steve French <sfrench@...ba.org>,
LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] cifs: retry lookup and readdir when EAGAIN is returned.
tentatively merged into cifs-2.6.git for-next pending testing and any
more review feedback
On Wed, Jun 16, 2021 at 1:09 AM ronnie sahlberg
<ronniesahlberg@...il.com> wrote:
>
> Looks good to me.
>
> Acked-by: Ronnie Sahlberg <lsahlber@...hat.com>
>
> On Wed, Jun 16, 2021 at 2:44 AM Thiago Rafael Becker <trbecker@...il.com> wrote:
> >
> > According to the investigation performed by Jacob Shivers at Red Hat,
> > cifs_lookup and cifs_readdir leak EAGAIN when the user session is
> > deleted on the server. Fix this issue by implementing a retry with
> > limits, as is implemented in cifs_revalidate_dentry_attr.
> >
> > Reproducer based on the work by Jacob Shivers:
> >
> > ~~~
> > $ cat readdir-cifs-test.sh
> > #!/bin/bash
> >
> > # Install and configure powershell and sshd on the windows
> > # server as descibed in
> > # https://docs.microsoft.com/en-us/windows-server/administration/openssh/openssh_overview
> > # This script uses expect(1)
> >
> > USER=dude
> > SERVER=192.168.0.2
> > RPATH=root
> > PASS='password'
> >
> > function debug_funcs {
> > for line in $@ ; do
> > echo "func $line +p" > /sys/kernel/debug/dynamic_debug/control
> > done
> > }
> >
> > function setup {
> > echo 1 > /proc/fs/cifs/cifsFYI
> > debug_funcs wait_for_compound_request \
> > smb2_query_dir_first cifs_readdir \
> > compound_send_recv cifs_reconnect_tcon \
> > generic_ip_connect cifs_reconnect \
> > smb2_reconnect_server smb2_reconnect \
> > cifs_readv_from_socket cifs_readv_receive
> > tcpdump -i eth0 -w cifs.pcap host 192.168.2.182 & sleep 5
> > dmesg -C
> > }
> >
> > function test_call {
> > if [[ $1 == 1 ]] ; then
> > tracer="strace -tt -f -s 4096 -o trace-$(date -Iseconds).txt"
> > fi
> > # Change the command here to anything apropriate
> > $tracer ls $2 > /dev/null
> > res=$?
> > if [[ $1 == 1 ]] ; then
> > if [[ $res == 0 ]] ; then
> > 1>&2 echo success
> > else
> > 1>&2 echo "failure ($res)"
> > fi
> > fi
> > }
> >
> > mountpoint /mnt > /dev/null || mount -t cifs -o username=$USER,pass=$PASS //$SERVER/$RPATH /mnt
> >
> > test_call 0 /mnt/
> >
> > /usr/bin/expect << EOF
> > set timeout 60
> >
> > spawn ssh $USER@...RVER
> >
> > expect "yes/no" {
> > send "yes\r"
> > expect "*?assword" { send "$PASS\r" }
> > } "*?assword" { send "$PASS\r" }
> >
> > expect ">" { send "powershell close-smbsession -force\r" }
> > expect ">" { send "exit\r" }
> > expect eof
> > EOF
> >
> > sysctl -w vm.drop_caches=2 > /dev/null
> > sysctl -w vm.drop_caches=2 > /dev/null
> >
> > setup
> >
> > test_call 1 /mnt/
> > ~~~
> >
> > Signed-off-by: Thiago Rafael Becker <trbecker@...il.com>
> > ---
> > fs/cifs/dir.c | 4 ++++
> > fs/cifs/smb2ops.c | 5 +++++
> > 2 files changed, 9 insertions(+)
> >
> > diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
> > index 6bcd3e8f7cda..7c641f9a3dac 100644
> > --- a/fs/cifs/dir.c
> > +++ b/fs/cifs/dir.c
> > @@ -630,6 +630,7 @@ cifs_lookup(struct inode *parent_dir_inode, struct dentry *direntry,
> > struct inode *newInode = NULL;
> > const char *full_path;
> > void *page;
> > + int retry_count = 0;
> >
> > xid = get_xid();
> >
> > @@ -673,6 +674,7 @@ cifs_lookup(struct inode *parent_dir_inode, struct dentry *direntry,
> > cifs_dbg(FYI, "Full path: %s inode = 0x%p\n",
> > full_path, d_inode(direntry));
> >
> > +again:
> > if (pTcon->posix_extensions)
> > rc = smb311_posix_get_inode_info(&newInode, full_path, parent_dir_inode->i_sb, xid);
> > else if (pTcon->unix_ext) {
> > @@ -687,6 +689,8 @@ cifs_lookup(struct inode *parent_dir_inode, struct dentry *direntry,
> > /* since paths are not looked up by component - the parent
> > directories are presumed to be good here */
> > renew_parental_timestamps(direntry);
> > + } else if (rc == -EAGAIN && retry_count++ < 10) {
> > + goto again;
> > } else if (rc == -ENOENT) {
> > cifs_set_time(direntry, jiffies);
> > newInode = NULL;
> > diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
> > index 21ef51d338e0..d241e6af8fe4 100644
> > --- a/fs/cifs/smb2ops.c
> > +++ b/fs/cifs/smb2ops.c
> > @@ -2325,6 +2325,7 @@ smb2_query_dir_first(const unsigned int xid, struct cifs_tcon *tcon,
> > struct smb2_query_directory_rsp *qd_rsp = NULL;
> > struct smb2_create_rsp *op_rsp = NULL;
> > struct TCP_Server_Info *server = cifs_pick_channel(tcon->ses);
> > + int retry_count = 0;
> >
> > utf16_path = cifs_convert_path_to_utf16(path, cifs_sb);
> > if (!utf16_path)
> > @@ -2372,10 +2373,14 @@ smb2_query_dir_first(const unsigned int xid, struct cifs_tcon *tcon,
> >
> > smb2_set_related(&rqst[1]);
> >
> > +again:
> > rc = compound_send_recv(xid, tcon->ses, server,
> > flags, 2, rqst,
> > resp_buftype, rsp_iov);
> >
> > + if (rc == -EAGAIN && retry_count++ < 10)
> > + goto again;
> > +
> > /* If the open failed there is nothing to do */
> > op_rsp = (struct smb2_create_rsp *)rsp_iov[0].iov_base;
> > if (op_rsp == NULL || op_rsp->sync_hdr.Status != STATUS_SUCCESS) {
> > --
> > 2.31.1
> >
--
Thanks,
Steve
Powered by blists - more mailing lists