netdev - [RFC PATCH net] splice: Do not set SPLICE_F_MORE flag if end of file is reached

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [day] [month] [year] [list]

Message-Id: <1591392508-14592-1-git-send-email-pooja.trivedi@stackpath.com>
Date:   Fri,  5 Jun 2020 21:28:28 +0000
From:   Pooja Trivedi <poojatrivedi@...il.com>
To:     pooja.trivedi@...ckpath.com, mallesh537@...il.com,
        josh.tway@...ckpath.com, viro@...iv.linux.org.uk, ast@...nel.org,
        kafai@...com, daniel@...earbox.net, songliubraving@...com,
        yhs@...com, andriin@...com, john.fastabend@...il.com,
        kpsingh@...omium.org, netdev@...r.kernel.org
Cc:     Mallesham Jatharkonda <mallesham.jatharkonda@...convergence.com>
Subject: [RFC PATCH net] splice: Do not set SPLICE_F_MORE flag if end of file is reached

SPLICE_F_MORE should only get set if read_len < len and pos does
not indicate file end. This is because the passed-in len can be
greater than file size and read_len < len could indicate that end
of file has been reached and there is no more pending data.

------

This issue was found during kTLS testing. Details as described in
https://lists.openwall.net/netdev/2020/06/02/146 are below:

When sendfile is used for kTLS file delivery and
the size provided to sendfile via its 'count'
parameter is greater than the file size, kTLS fails
to send the file correctly. The last chunk of the
file is not sent, and the data integrity of the
file is compromised on the receiver side.
Based on studying the sendfile source code, in
such a case, last chunk of the file will be passed
with the MSG_MORE flag set. Following snippet from
fs/splice.c:1814 shows code within the while loop
in splice_direct_to_actor() function that sets this
flag:

--------

	/*
	 * If more data is pending, set SPLICE_F_MORE
	 * If this is the last data and SPLICE_F_MORE
	 * was not set initially, clears it.
	 */
	if (read_len < len)
		sd->flags |= SPLICE_F_MORE;
	else if (!more)
		sd->flags &= ~SPLICE_F_MORE;

--------

Due to this, tls layer adds the chunk to the pending
records, but does not push it. Following lines of code
from tls_sw_do_sendpage() function in tls_sw.c:1153 show
the end of record (eor) variable being set based on
MSG_MORE flag:

--------

	bool eor;

	eor = !(flags & (MSG_MORE | MSG_SENDPAGE_NOTLAST));

--------

This eor bool is then used in the condition check for
full_record, end of record, or sk_msg_full in
tls_sw_do_sendpage() function in tls_sw.c:1212:

--------

	if (full_record || eor || sk_msg_full(msg_pl)) {
		ret = bpf_exec_tx_verdict(msg_pl, sk, full_record,
				  record_type, &copied, flags);
		if (ret) {
			if (ret == -EINPROGRESS)
				num_async++;
			else if (ret == -ENOMEM)
				goto wait_for_memory;
			else if (ret != -EAGAIN) {
				if (ret == -ENOSPC)
					ret = 0;
				goto sendpage_end;
			}
		}
	}
	continue;

--------

Changing the code in splice_direct_to_actor() function
in fs/splice.c to detect end of file by checking 'pos'
variable against file size, and setting MSG_MORE flag
only when EOF is not reached, fixes the issue:

In splice_direct_to_actor():988:

                 * If this is the last data and SPLICE_F_MORE was not set
                 * initially, clears it.
                 */
-               if (read_len < len)
-                       sd->flags |= SPLICE_F_MORE;
-               else if (!more)
+               if (read_len < len) {
+                       if (pos < i_size_read(file_inode(in)))
+                               sd->flags |= SPLICE_F_MORE;
+               } else if (!more)
                        sd->flags &= ~SPLICE_F_MORE;
+               }

-------

Here is the kTLS selftest that was submitted, and that helps
reproduce the issue: https://lists.openwall.net/netdev/2020/06/05/109

-------

Signed-off-by: Pooja Trivedi <pooja.trivedi@...ckpath.com>
Signed-off-by: Mallesham Jatharkonda<mallesham.jatharkonda@...convergence.com>
Signed-off-by: Josh Tway <josh.tway@...ckpath.com>
---
 fs/splice.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/fs/splice.c b/fs/splice.c
index 6b3c9a0..6408393 100644
--- a/fs/splice.c
+++ b/fs/splice.c
@@ -959,10 +959,12 @@ ssize_t splice_direct_to_actor(struct file *in, struct splice_desc *sd,
 		 * If this is the last data and SPLICE_F_MORE was not set
 		 * initially, clears it.
 		 */
-		if (read_len < len)
-			sd->flags |= SPLICE_F_MORE;
-		else if (!more)
+		if (read_len < len) {
+			if (pos < i_size_read(file_inode(in)))
+				sd->flags |= SPLICE_F_MORE;
+		} else if (!more) {
 			sd->flags &= ~SPLICE_F_MORE;
+		}
 		/*
 		 * NOTE: nonblocking mode only applies to the input. We
 		 * must not do the output in nonblocking mode as then we
-- 
1.8.3.1