netdev - Re: [RFC PATCH net 1/1] net/tls(TLS_SW): Add selftest for 'chunked' sendfile test

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>] [<thread-prev] [day] [month] [year] [list]

Date:   Wed, 3 Jun 2020 13:58:37 -0400
From:   Pooja Trivedi <poojatrivedi@...il.com>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     netdev@...r.kernel.org, linux-crypto@...r.kernel.org
Subject: Re: [RFC PATCH net 1/1] net/tls(TLS_SW): Add selftest for 'chunked'
 sendfile test

On Tue, Jun 2, 2020 at 3:19 PM Jakub Kicinski <kuba@...nel.org> wrote:
>
> On Tue,  2 Jun 2020 14:56:25 +0000 Pooja Trivedi wrote:
> > This selftest tests for cases where sendfile's 'count'
> > parameter is provided with a size greater than the intended
> > file size.
> >
> > Motivation: When sendfile is provided with 'count' parameter
> > value that is greater than the size of the file, kTLS example
> > fails to send the file correctly. Last chunk of the file is
> > not sent, and the data integrity is compromised.
> > The reason is that the last chunk has MSG_MORE flag set
> > because of which it gets added to pending records, but is
> > not pushed.
> > Note that if user space were to send SSL_shutdown control
> > message, pending records would get flushed and the issue
> > would not happen. So a shutdown control message following
> > sendfile can mask the issue.
> >
> > Signed-off-by: Pooja Trivedi <pooja.trivedi@...ckpath.com>
>
> Looks good, thanks. Did you submit the change to splice officially?
> We'd need to get an Ack from VFS folks on it (Al Viro, probably?)
> or even merge it via the vfs tree.
>

No, I did not submit the change to splice yet. I can do that next.
I wanted to first run this through here and hear thoughts/suggestions.

> Minor nits below.
>

Will change and resubmit the selftest. Thanks.

> > diff --git a/tools/testing/selftests/net/tls.c b/tools/testing/selftests/net/tls.c
> > index 0ea44d9..f0455e6 100644
> > --- a/tools/testing/selftests/net/tls.c
> > +++ b/tools/testing/selftests/net/tls.c
> > @@ -198,6 +198,64 @@
> >       EXPECT_EQ(recv(self->cfd, buf, st.st_size, MSG_WAITALL), st.st_size);
> >  }
> >
> > +static void chunked_sendfile(struct __test_metadata *_metadata,
> > +                          struct _test_data_tls *self,
> > +                          uint16_t chunk_size,
> > +                          uint16_t extra_payload_size)
> > +{
> > +     char buf[TLS_PAYLOAD_MAX_LEN];
> > +     uint16_t test_payload_size;
> > +     int size = 0;
> > +     int ret;
> > +     char tmpfile[] = ".TMP_ktls";
>
> Could we place the file in /tmp and use mktemp()? I sometimes run the
> selftests from a read-only NFS mount, and trying to create a file in
> current dir breaks that.
>
> > +     int fd = open(tmpfile, O_RDWR | O_CREAT | O_TRUNC, 0644);
>
> We can unlink right after we open. The file won't get removed as long
> as we have a reference to it, and we minimize the risk of leaving it
> behind.
>
> > +     off_t offset = 0;
> > +
> > +     ASSERT_GE(fd, 0);
> > +     EXPECT_GE(chunk_size, 1);
> > +     test_payload_size = chunk_size + extra_payload_size;
> > +     ASSERT_GE(TLS_PAYLOAD_MAX_LEN, test_payload_size);
> > +     memset(buf, 1, test_payload_size);
> > +     size = write(fd, buf, test_payload_size);
> > +     EXPECT_EQ(size, test_payload_size);
> > +     fsync(fd);
> > +
> > +     while (size > 0) {
> > +             ret = sendfile(self->fd, fd, &offset, chunk_size);
> > +             EXPECT_GE(ret, 0);
> > +             size -= ret;
> > +     }
> > +
> > +     EXPECT_EQ(recv(self->cfd, buf, test_payload_size, MSG_WAITALL),
> > +               test_payload_size);
> > +
> > +     close(fd);
> > +     unlink(tmpfile);
> > +}
>