ACK: [SRU][Mantic][PATCH 0/1] smb: wsize blocks of bytes followed with binary zeros on copy, destroying data

[Tim Gardner]

On 2/21/24 5:50 PM, Matthew Ruffell wrote:
> BugLink: https://bugs.launchpad.net/bugs/2049634
> 
> [Impact]
> 
> Upon installing the 6.5 HWE kernel on Jammy, users with a custom wsize set will
> see data destruction when copying files from their systems onto a cifs smb mount.
> 
> wsize defaults to 65535 bytes, but when set to smaller values, like 16850, users
> will see blocks of 16850 bytes copied over, followed by 3900 binary zeros,
> followed by the next block of data followed by more binary zeros. This destroys
> what you are copying, and the data corruption is completely silent.
> 
> A workaround is to set wsize to a multiple of PAGE_SIZE.
> 
> [Fix]
> 
> This was fixed upstream in 6.8-rc5 by the following:
> 
> commit 4860abb91f3d7fbaf8147d54782149bb1fc45892
> Author: Steve French <stfrench at microsoft.com>
> Date: Tue Feb 6 16:34:22 2024 -0600
> Subject: smb: Fix regression in writes when non-standard maximum write size negotiated
> Link: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=4860abb91f3d7fbaf8147d54782149bb1fc45892
> 
> This patch has been selected for upstream stable.
> 
> The patch looks at wsize negotiation from the server, and also what is set on
> the mount command line, and if not a multiple of PAGE_SIZE, rounds it down,
> taking care to not be 0.
> The real corruption bug still exists in netfs/folios subsystems and we are
> looking for it still, but this solves the immediate data corruption issues on
> smb.
> 
> [Testcase]
> 
> Start two VMs, one for the server, and the other, the client.
> 
> Server
> ------
> 
> $ sudo apt update
> $ sudo apt upgrade
> $ sudo apt install samba
> $ sudo vim /etc/samba/smb.conf
> server min protocol = NT1
> [sambashare]
>      comment = Samba on Ubuntu
>      path = /home/ubuntu/sambashare
>      read only = no
>      browsable = yes
> $ mkdir ~/sambashare
> $ sudo smbpasswd -a ubuntu
> 
> Client
> ------
> 
> $ sudo apt update
> $ sudo apt install cifs-utils
> $ mkdir ~/share
> $ sudo mount -t cifs -o username=ubuntu,vers=1.0,wsize=16850 //192.168.122.172/sambashare ~/share
> $ ( set -o pipefail && head --bytes=$(( 55 * 1000 )) /dev/zero | openssl enc -aes-128-ctr -nosalt -pass "pass:my-seed" -iter 1 | hexdump --no-squeezing --format '40/1 "%02x"' --format '"\n"' >"testdata.txt" )
> $ sha256sum testdata.txt
> 9ec09af020dce3270ea76531141940106f173c7243b7193a553480fb8500b3f2  testdata.txt
> 
> Now copy the file to the share.
> 
> Client
> ------
> $ cp testdata.txt share/
> 
> Server
> ------
> $ sha256sum sambashare/testdata.txt
> 9e573a0aa795f9cd4de4ac684a1c056dbc7d2ba5494d02e71b6225ff5f0fd866  sambashare/testdata.txt
> 
> The SHA256 hash is different. If you view the file with less, you will find a
> block of wsize=16850 bytes, then 3900 bytes of binary zeros, followed by another
> wsize=16850 bytes, then 3900 bytes of binary zeros, etc.
> 
> An example of a broken file is:
> https://launchpadlibrarian.net/712573213/testdata-back-from-server.txt
> 
> [Where problems could occur]
> 
> We are changing the wsize that the SMB server negotiates or the user explicitly
> sets on the mount command line to a safe value, if the asked for value was not
> a multiple of PAGE_SIZE.
> 
> It is unlikely that any users will notice any difference. If the wsize happens
> to be rounded down to a significantly smaller number, there may be a small
> performance impact, e.g. you set wsize=8094 for some reason, it would round down
> to wsize=4096, and lead to double the writes. At least you have data integrity
> though.
> 
> Most users default to wsize=65535, and will have no impact at all. Only those
> with very old smb 1.0 servers that negotiate down to 16850 will see any
> difference.
> 
> If a regression were to occur, it could result in user's wsize being incorrectly
> set, leading to the underlying netfs/folios data corruption being triggered and
> causing data corruption.
> 
> [Other info]
> 
> The issue was introduced in 6.3-rc1 by:
> 
> commit d08089f649a0cfb2099c8551ac47eef0cc23fdf2
> Author: David Howells <dhowells at redhat.com>
> Date: Mon Jan 24 21:13:24 2022 +0000
> Subject: cifs: Change the I/O paths to use an iterator rather than a page list
> Link: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=d08089f649a0cfb2099c8551ac47eef0cc23fdf2
> 
> Upstream mailing list discussion:
> 
> https://lore.kernel.org/linux-cifs/6a65b2d1-7596-438a-8ade-2f7526b15596@rd10.de/T/#m22cd9b7289f87cd945978bd7995bcaf1beebfe67
> 
> Steve French (1):
>    smb: Fix regression in writes when non-standard maximum write size
>      negotiated
> 
>   fs/smb/client/connect.c    | 14 ++++++++++++--
>   fs/smb/client/fs_context.c | 11 +++++++++++
>   2 files changed, 23 insertions(+), 2 deletions(-)
> 
Acked-by: Tim Gardner <tim.gardner at canonical.com>
-- 
-----------
Tim Gardner
Canonical, Inc