10 Jul, 2021
3 commits
-
To 2.33
Signed-off-by: Steve French
-
The optional @ref parameter might contain an NULL node_name, so
prevent dereferencing it in cifs_compose_mount_options().Addresses-Coverity: 1476408 ("Explicit null dereferenced")
Signed-off-by: Paulo Alcantara (SUSE)
Signed-off-by: Steve French -
Support for faster packet signing (using GMAC instead of CMAC) can
now be negotiated to some newer servers, including Windows.
See MS-SMB2 section 2.2.3.17.This patch adds support for sending the new negotiate context
with the first of three supported signing algorithms (AES-CMAC)
and decoding the response. A followon patch will add support
for sending the other two (including AES-GMAC, which is fastest)
and changing the signing algorithm used based on what was
negotiated.To allow the client to request GMAC signing set module parameter
"enable_negotiate_signing" to 1.Reviewed-by: Ronnie Sahlberg
Reviewed-by: Pavel Shilovsky
Signed-off-by: Steve French
09 Jul, 2021
1 commit
-
Use the nice helpers to initialize and the uid/gid/cred_uid when passed as mount arguments.
Signed-off-by: Ronnie Sahlberg
Acked-by: Pavel Shilovsky
Signed-off-by: Steve French
08 Jul, 2021
4 commits
-
Coverity also complains about the way we calculate the offset
(starting from the address of a 4 byte array within the
header structure rather than from the beginning of the struct
plus 4 bytes) for SMB1 PosixLock. This changeset
doesn't change the address but makes it slightly clearer.Addresses-Coverity: 711520 ("Out of bounds write")
Reviewed-by: Paulo Alcantara (SUSE)
Signed-off-by: Steve French -
Coverity also complains about the way we calculate the offset
(starting from the address of a 4 byte array within the
header structure rather than from the beginning of the struct
plus 4 bytes) for SMB1 RenameOpenFile. This changeset
doesn't change the address but makes it slightly clearer.Addresses-Coverity: 711521 ("Out of bounds write")
Reviewed-by: Paulo Alcantara (SUSE)
Signed-off-by: Steve French -
Coverity also complains about the way we calculate the offset
(starting from the address of a 4 byte array within the
header structure rather than from the beginning of the struct
plus 4 bytes) for SMB1 SetFileDisposition (which is used to
unlink a file by setting the delete on close flag). This
changeset doesn't change the address but makes it slightly
clearer.Addresses-Coverity: 711524 ("Out of bounds write")
Reviewed-by: Paulo Alcantara (SUSE)
Signed-off-by: Steve French -
Coverity also complains about the way we calculate the offset
(starting from the address of a 4 byte array within the header
structure rather than from the beginning of the struct plus
4 bytes) for setting the file size using SMB1. This changeset
doesn't change the address but makes it slightly clearer.Addresses-Coverity: 711525 ("Out of bounds write")
Reviewed-by: Paulo Alcantara (SUSE)
Signed-off-by: Steve French
05 Jul, 2021
1 commit
-
Although it compiles, the test robot correctly noted:
'cifsacl.h' file not found with include; use "quotes" insteadReported-by: kernel test robot
Signed-off-by: Steve French
03 Jul, 2021
4 commits
-
Coverity also complains about the way we calculate the offset
(starting from the address of a 4 byte array within the
header structure rather than from the beginning of the struct
plus 4 bytes) for doing SetPathInfo (setattr) when using the Unix
extensions. This doesn't change the address but makes it
slightly clearer.Addresses-Coverity: 711528 ("Out of bounds read")
Reviewed-by: Ronnie Sahlberg
Signed-off-by: Steve French -
Coverity also complains about the way we calculate the offset
(starting from the address of a 4 byte array within the
header structure rather than from the beginning of the struct
plus 4 bytes) for creating SMB1 symlinks when using the Unix
extensions. This doesn't change the address but
makes it slightly clearer.Addresses-Coverity: 711530 ("Out of bounds read")
Reviewed-by: Ronnie Sahlberg
Signed-off-by: Steve French -
Coverity complains about the way we calculate the offset
(starting from the address of a 4 byte array within the
header structure rather than from the beginning of the struct
plus 4 bytes). This doesn't change the address but
makes it slightly clearer.Addresses-Coverity: 711529 ("Out of bounds read")
Reviewed-by: Ronnie Sahlberg
Signed-off-by: Steve French -
There were three places where we were not taking the spinlock
around updates to server->tcpStatus when it was being modified.
To be consistent (also removes Coverity warning) and to remove
possibility of race best to lock all places where it is updated.
Two of the three were in initialization of the field and can't
race - but added lock around the other.Addresses-Coverity: 1399512 ("Data race condition")
Reviewed-by: Paulo Alcantara (SUSE)
Signed-off-by: Steve French
26 Jun, 2021
1 commit
-
There was one place where we weren't locking CurrentMid, and although
likely to be safe since even without the lock since it is during
negotiate protocol, it is more consistent to lock it in this last remaining
place, and avoids confusing Coverity warning.Addresses-Coverity: 1486665 ("Data race condition")
Signed-off-by: Steve French
25 Jun, 2021
1 commit
-
In the other places where we update ses->status we protect the
updates via GlobalMid_Lock. So to be consistent add the same
locking around it in cifs_put_smb_ses where it was missing.Addresses-Coverity: 1268904 ("Data race condition")
Signed-off-by: Steve French
24 Jun, 2021
6 commits
-
We weren't checking if tcon is null before setting dfs path,
although we check for null tcon in an earlier assignment statement.Addresses-Coverity: 1476411 ("Dereference after null check")
Signed-off-by: Steve French -
dacl_ptr can be null so we must check for it everywhere it is
used in build_sec_desc.Addresses-Coverity: 1475598 ("Explicit null dereference")
Signed-off-by: Steve French -
in cifs_do_create we check if newinode is valid before referencing it
but are missing the check in one place in fs/cifs/dir.cAddresses-Coverity: 1357292 ("Dereference after null check")
Acked-by: Sachin Prabhu
Signed-off-by: Steve French -
In both these cases sid_to_id unconditionally returned success, and
used the default uid/gid for the mount, so setting rc is confusing
and simply gets overwritten (set to 0) later in the function.Addresses-Coverity: 1491672 ("Unused value")
Signed-off-by: Steve French -
The recently updated MS-SMB2 (June 2021) added protocol definitions
for a new level 60 for query directory (FileIdExtdDirectoryInformation).Signed-off-by: Steve French
-
This code sets "ses" to NULL which will lead to a NULL dereference on
the second iteration through the loop.Fixes: 85346c17e425 ("cifs: convert list_for_each to entry variant in smb2misc.c")
Signed-off-by: Dan Carpenter
Reviewed-by: Aurelien Aptel
Signed-off-by: Steve French
23 Jun, 2021
1 commit
-
There were two places where we weren't checking for error
(e.g. ERESTARTSYS) while waiting for rdma resolution.Addresses-Coverity: 1462165 ("Unchecked return value")
Reviewed-by: Tom Talpey
Reviewed-by: Long Li
Signed-off-by: Steve French
22 Jun, 2021
1 commit
-
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memcpy(), memmove(), and memset(), avoid
intentionally reading across neighboring fields.Instead of using memcpy to read across multiple struct members, just
perform per-member assignments as already done for other members.Signed-off-by: Kees Cook
Signed-off-by: Steve French
21 Jun, 2021
17 commits
-
Although we may need this in some cases in the future, remove the
currently unused, non-compounded version of POSIX query info,
SMB11_posix_query_info (instead smb311_posix_query_path_info is now
called e.g. when revalidating dentries or retrieving info for getattr)Addresses-Coverity: 1495708 ("Resource leaks")
Signed-off-by: Steve French -
We were trying to fill in uninitialized file attributes in the error case.
Addresses-Coverity: 139689 ("Uninitialized variables")
Signed-off-by: Steve French -
Although in practice this can not occur (since IPv4 and IPv6 are the
only two cases currently supported), it is cleaner to avoid uninitialized
variable warnings.Addresses smatch warning:
fs/cifs/cifs_swn.c:468 cifs_swn_store_swn_addr() error: uninitialized symbol 'port'.Reported-by: kernel test robot
Reported-by: Dan Carpenter
CC: Samuel Cabrero
Signed-off-by: Steve French -
tcon can not be null in SMB2_tcon function so the check
is not relevant and removing it makes Coverity happy.Acked-by: Ronnie Sahlberg
Addresses-Coverity: 13250131 ("Dereference before null check")
Signed-off-by: Steve French -
Add SPDX license identifier and replace license boilerplate.
Corrects various checkpatch errors with the older format for
noting the LGPL license.Signed-off-by: Steve French
-
convert list_for_each() to list_for_each_entry() where
applicable.Reported-by: Hulk Robot
Signed-off-by: Baokun Li
Signed-off-by: Steve French -
convert list_for_each() to list_for_each_entry() where
applicable.Reported-by: Hulk Robot
Signed-off-by: Baokun Li
Signed-off-by: Steve French -
In posix_info_parse() we call posix_info_sid_size twice for each of the owner and the group
sid. The first time to check that it is valid, i.e. >= 0 and the second time
to just pass it in as a length to memcpy().
As this is a pure function we know that it can not be negative the second time and this
is technically a false warning in coverity.
However, as it is a pure function we are just wasting cycles by calling it a second time.
Record the length from the first time we call it and save some cycles as well as make
Coverity happy.Addresses-Coverity-ID: 1491379 ("Argument can not be negative")
Signed-off-by: Ronnie Sahlberg
Signed-off-by: Steve French -
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 appropriate
$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 60spawn ssh $USER@$SERVER
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
EOFsysctl -w vm.drop_caches=2 > /dev/null
sysctl -w vm.drop_caches=2 > /dev/nullsetup
test_call 1 /mnt/
~~~Signed-off-by: Thiago Rafael Becker
Acked-by: Ronnie Sahlberg
Signed-off-by: Steve French -
Interlink is a special type of DFS link that resolves to a different
DFS domain-based namespace. To determine whether it is an interlink
or not, check if ReferralServers and StorageServers bits are set to 1
and 0 respectively in ReferralHeaderFlags, as specified in MS-DFSC
3.1.5.4.5 Determining Whether a Referral Response is an Interlink.Signed-off-by: Paulo Alcantara (SUSE)
Reviewed-by: Aurelien Aptel
Signed-off-by: Steve French -
Decode negTokenInit with lib/asn1_decoder. For that,
add OIDs in linux/oid_registry.h and a negTokenInit
ASN1 file, "spnego_negtokeninit.asn1".
And define decoder's callback functions, which
are the gssapi_this_mech for checking SPENGO oid and
the neg_token_init_mech_type for getting authentication
mechanisms supported by a server.Signed-off-by: Hyunchul Lee
Reviewed-by: Aurelien Aptel
Reviewed-by: Paulo Alcantara (SUSE)
Signed-off-by: Steve French -
When refreshing the DFS cache, keep SMB2 IOCTL calls as much outside
critical sections as possible and avoid read/write starvation when
getting new DFS referrals by using broken or slow connections.Signed-off-by: Paulo Alcantara (SUSE)
Reviewed-by: Aurelien Aptel
Signed-off-by: Steve French -
CONFIG_CIFS_STATS2 can be very useful since it shows
latencies by command, and allows enabling the slow response
dynamic tracepoint which can be useful to identify
performance problems.For example:
Total time spent processing by command. Time units are jiffies (1000 per second)
SMB3 CMD Number Total Time Fastest Slowest
-------- ------ ---------- ------- -------
0 1 2 2 2
1 2 6 2 4
2 0 0 0 0
3 4 11 2 4
4 2 16 5 11
5 4546 34104 2 487
6 4421 32901 2 487
7 0 0 0 0
8 695 2781 2 39
9 391 1708 2 27
10 0 0 0 0
11 4 6 1 2
12 0 0 0 0
13 0 0 0 0
14 3887 17696 0 128
15 0 0 0 0
16 1471 9950 1 487
17 169 2695 9 116
18 80 381 2 10
1 2 6 2 4
2 0 0 0 0
3 4 11 2 4
4 2 16 5 11
5 4546 34104 2 487
6 4421 32901 2 487
7 0 0 0 0
8 695 2781 2 39
9 391 1708 2 27
10 0 0 0 0
11 4 6 1 2
12 0 0 0 0
13 0 0 0 0
14 3887 17696 0 128
15 0 0 0 0
16 1471 9950 1 487
17 169 2695 9 116
18 80 381 2 10Reviewed-by: Aurelien Aptel
Reviewed-by: Paulo Alcantara (SUSE)
Signed-off-by: Steve French -
When we lookup an smb session based on session id,
we did not up the ref-count for the session. This can
potentially cause issues if the session is freed from under us.Signed-off-by: Shyam Prasad N
Reviewed-by: Aurelien Aptel
Reviewed-by: Paulo Alcantara (SUSE)
Signed-off-by: Steve French -
It isn't enough to have unshared tcons because multiple DFS mounts can
connect to same target server and failover to different servers, so we
can't use a single tcp server for such cases.For the simplest solution, use nosharesock option to achieve that.
Signed-off-by: Paulo Alcantara (SUSE)
Reviewed-by: Aurelien Aptel
Signed-off-by: Steve French -
We don't want to refresh the dfs cache in very short intervals, so
setting a minimum interval of 2 minutes is OK.If it needs to be refreshed immediately, one could have the cache
cleared with$ echo 0 > /proc/fs/cifs/dfscache
and then remounting the dfs share.
Signed-off-by: Paulo Alcantara (SUSE)
Reviewed-by: Aurelien Aptel
Signed-off-by: Steve French -
Fix cache lookup and hash calculations when handling paths with
different cases.Signed-off-by: Paulo Alcantara (SUSE)
Reviewed-by: Aurelien Aptel
Signed-off-by: Steve French
