| 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: <e86656c2-079b-5223-cc69-d651cb846e24@sberdevices.ru>
Date: Mon, 21 Nov 2022 16:49:23 +0000
From: Arseniy Krasnov <AVKrasnov@...rdevices.ru>
To: Stefano Garzarella <sgarzare@...hat.com>
CC: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"virtualization@...ts.linux-foundation.org"
<virtualization@...ts.linux-foundation.org>,
kernel <kernel@...rdevices.ru>,
Bobby Eshleman <bobby.eshleman@...il.com>,
Krasnov Arseniy <oxffffaa@...il.com>
Subject: Re: [RFC PATCH v1 1/3] test/vsock: rework message bound test
On 21.11.2022 17:46, Stefano Garzarella wrote:
> On Tue, Nov 15, 2022 at 08:50:36PM +0000, Arseniy Krasnov wrote:
>> This updates message bound test making it more complex. Instead of
>> sending 1 bytes messages with one MSG_EOR bit, it sends messages of
>> random length(one half of messages are smaller than page size, second
>> half are bigger) with random number of MSG_EOR bits set. Receiver
>> also don't know total number of messages.
>>
>> Signed-off-by: Arseniy Krasnov <AVKrasnov@...rdevices.ru>
>> ---
>> tools/testing/vsock/control.c | 34 +++++++++
>> tools/testing/vsock/control.h | 2 +
>> tools/testing/vsock/util.c | 13 ++++
>> tools/testing/vsock/util.h | 1 +
>> tools/testing/vsock/vsock_test.c | 115 +++++++++++++++++++++++++++----
>> 5 files changed, 152 insertions(+), 13 deletions(-)
>>
>> diff --git a/tools/testing/vsock/control.c b/tools/testing/vsock/control.c
>> index 4874872fc5a3..bed1649bdf3d 100644
>> --- a/tools/testing/vsock/control.c
>> +++ b/tools/testing/vsock/control.c
>> @@ -141,6 +141,40 @@ void control_writeln(const char *str)
>> timeout_end();
>> }
>>
>> +void control_writeulong(unsigned long value)
>> +{
>> + char str[32];
>> +
>> + if (snprintf(str, sizeof(str), "%lu", value) >= sizeof(str)) {
>> + perror("snprintf");
>> + exit(EXIT_FAILURE);
>> + }
>> +
>> + control_writeln(str);
>> +}
>> +
>> +unsigned long control_readulong(bool *ok)
>> +{
>> + unsigned long value;
>> + char *str;
>> +
>> + if (ok)
>> + *ok = false;
>> +
>> + str = control_readln();
>> +
>> + if (str == NULL)
>
> checkpatch suggests to use !str
>
>> + return 0;
>
> Maybe we can just call exit(EXIT_FAILURE) here and remove the `ok`
> parameter, since I'm not sure we can recover from this error.
>
>> +
>> + value = strtoul(str, NULL, 10);
>> + free(str);
>> +
>> + if (ok)
>> + *ok = true;
>> +
>> + return value;
>> +}
>> +
>> /* Return the next line from the control socket (without the trailing newline).
>> *
>> * The program terminates if a timeout occurs.
>> diff --git a/tools/testing/vsock/control.h b/tools/testing/vsock/control.h
>> index 51814b4f9ac1..cdd922dfea68 100644
>> --- a/tools/testing/vsock/control.h
>> +++ b/tools/testing/vsock/control.h
>> @@ -9,7 +9,9 @@ void control_init(const char *control_host, const char *control_port,
>> void control_cleanup(void);
>> void control_writeln(const char *str);
>> char *control_readln(void);
>> +unsigned long control_readulong(bool *ok);
>> void control_expectln(const char *str);
>> bool control_cmpln(char *line, const char *str, bool fail);
>> +void control_writeulong(unsigned long value);
>>
>> #endif /* CONTROL_H */
>> diff --git a/tools/testing/vsock/util.c b/tools/testing/vsock/util.c
>> index 2acbb7703c6a..351903836774 100644
>> --- a/tools/testing/vsock/util.c
>> +++ b/tools/testing/vsock/util.c
>> @@ -395,3 +395,16 @@ void skip_test(struct test_case *test_cases, size_t test_cases_len,
>>
>> test_cases[test_id].skip = true;
>> }
>> +
>> +unsigned long djb2(const void *data, size_t len)
>
> I would add hash_ as a prefix (or suffix).
>
>> +{
>> + unsigned long hash = 5381;
>> + int i = 0;
>> +
>> + while (i < len) {
>> + hash = ((hash << 5) + hash) + ((unsigned char *)data)[i];
>> + i++;
>> + }
>> +
>> + return hash;
>> +}
>> diff --git a/tools/testing/vsock/util.h b/tools/testing/vsock/util.h
>> index a3375ad2fb7f..988cc69a4642 100644
>> --- a/tools/testing/vsock/util.h
>> +++ b/tools/testing/vsock/util.h
>> @@ -49,4 +49,5 @@ void run_tests(const struct test_case *test_cases,
>> void list_tests(const struct test_case *test_cases);
>> void skip_test(struct test_case *test_cases, size_t test_cases_len,
>> const char *test_id_str);
>> +unsigned long djb2(const void *data, size_t len);
>> #endif /* UTIL_H */
>> diff --git a/tools/testing/vsock/vsock_test.c b/tools/testing/vsock/vsock_test.c
>> index bb6d691cb30d..107c11165887 100644
>> --- a/tools/testing/vsock/vsock_test.c
>> +++ b/tools/testing/vsock/vsock_test.c
>> @@ -284,10 +284,14 @@ static void test_stream_msg_peek_server(const struct test_opts *opts)
>> close(fd);
>> }
>>
>> -#define MESSAGES_CNT 7
>> -#define MSG_EOR_IDX (MESSAGES_CNT / 2)
>> +#define SOCK_BUF_SIZE (2 * 1024 * 1024)
>> +#define MAX_MSG_SIZE (32 * 1024)
>> +
>> static void test_seqpacket_msg_bounds_client(const struct test_opts *opts)
>> {
>> + unsigned long curr_hash;
>> + int page_size;
>> + int msg_count;
>> int fd;
>>
>> fd = vsock_seqpacket_connect(opts->peer_cid, 1234);
>> @@ -296,18 +300,69 @@ static void test_seqpacket_msg_bounds_client(const struct test_opts *opts)
>> exit(EXIT_FAILURE);
>> }
>>
>> - /* Send several messages, one with MSG_EOR flag */
>> - for (int i = 0; i < MESSAGES_CNT; i++)
>> - send_byte(fd, 1, (i == MSG_EOR_IDX) ? MSG_EOR : 0);
>> + /* Wait, until receiver sets buffer size. */
>> + control_expectln("SRVREADY");
>> +
>> + curr_hash = 0;
>> + page_size = getpagesize();
>> + msg_count = SOCK_BUF_SIZE / MAX_MSG_SIZE;
>> +
>> + for (int i = 0; i < msg_count; i++) {
>> + ssize_t send_size;
>> + size_t buf_size;
>> + int flags;
>> + void *buf;
>> +
>> + /* Use "small" buffers and "big" buffers. */
>> + if (i & 1)
>> + buf_size = page_size +
>> + (rand() % (MAX_MSG_SIZE - page_size));
>> + else
>> + buf_size = 1 + (rand() % page_size);
>> +
>> + buf = malloc(buf_size);
>> +
>> + if (!buf) {
>> + perror("malloc");
>> + exit(EXIT_FAILURE);
>> + }
>> +
>> + /* Set at least one MSG_EOR + some random. */
>> + if (i == (msg_count / 2) || (rand() & 1)) {
>> + flags = MSG_EOR;
>> + curr_hash++;
>> + } else {
>> + flags = 0;
>> + }
>> +
>> + send_size = send(fd, buf, buf_size, flags);
>> +
>> + if (send_size < 0) {
>> + perror("send");
>> + exit(EXIT_FAILURE);
>> + }
>> +
>> + if (send_size != buf_size) {
>> + fprintf(stderr, "Invalid send size\n");
>> + exit(EXIT_FAILURE);
>> + }
>> +
>> + curr_hash += send_size;
>> + curr_hash = djb2(&curr_hash, sizeof(curr_hash));
>> + }
>>
>> control_writeln("SENDDONE");
>> + control_writeulong(curr_hash);
>
> Why do we need to hash the size?
>
> Maybe we can send it without making the hash, anyway even if it wraps,
> it should wrap the same way in both the server and the client.
> (Or maybe we can use uin32_t or uint64_t to make sure both were
> using 4 or 8 bytes)
Hello, thanks for review. I think if we will use sum of message size(IIUC), in most
paranoic case it won't guarantee message bounds control: single 4 bytes message
could be read as 4 x 1 byte message(IIUC of course). Idea of hashing is simple:
every iteration we do current_hash = hash(previous_hash + size of current message);
I think this is more reliable and protects from case described above.
All other comments - ack.
>
>> close(fd);
>> }
>>
>> static void test_seqpacket_msg_bounds_server(const struct test_opts *opts)
>> {
>> + unsigned long sock_buf_size;
>> + unsigned long remote_hash;
>> + unsigned long curr_hash;
>> int fd;
>> - char buf[16];
>> + char buf[MAX_MSG_SIZE];
>> struct msghdr msg = {0};
>> struct iovec iov = {0};
>>
>> @@ -317,25 +372,58 @@ static void test_seqpacket_msg_bounds_server(const struct test_opts *opts)
>> exit(EXIT_FAILURE);
>> }
>>
>> + sock_buf_size = SOCK_BUF_SIZE;
>> +
>> + if (setsockopt(fd, AF_VSOCK, SO_VM_SOCKETS_BUFFER_MAX_SIZE,
>> + &sock_buf_size, sizeof(sock_buf_size))) {
>> + perror("getsockopt");
>> + exit(EXIT_FAILURE);
>> + }
>> +
>> + if (setsockopt(fd, AF_VSOCK, SO_VM_SOCKETS_BUFFER_SIZE,
>> + &sock_buf_size, sizeof(sock_buf_size))) {
>> + perror("getsockopt");
>> + exit(EXIT_FAILURE);
>> + }
>> +
>> + /* Ready to receive data. */
>> + control_writeln("SRVREADY");
>> + /* Wait, until peer sends whole data. */
>> control_expectln("SENDDONE");
>> iov.iov_base = buf;
>> iov.iov_len = sizeof(buf);
>> msg.msg_iov = &iov;
>> msg.msg_iovlen = 1;
>>
>> - for (int i = 0; i < MESSAGES_CNT; i++) {
>> - if (recvmsg(fd, &msg, 0) != 1) {
>> - perror("message bound violated");
>> - exit(EXIT_FAILURE);
>> - }
>> + curr_hash = 0;
>>
>> - if ((i == MSG_EOR_IDX) ^ !!(msg.msg_flags & MSG_EOR)) {
>> - perror("MSG_EOR");
>> + while (1) {
>> + ssize_t recv_size;
>> +
>> + recv_size = recvmsg(fd, &msg, 0);
>> +
>> + if (!recv_size)
>> + break;
>> +
>> + if (recv_size < 0) {
>> + perror("recvmsg");
>> exit(EXIT_FAILURE);
>> }
>> +
>> + if (msg.msg_flags & MSG_EOR)
>> + curr_hash++;
>> +
>> + curr_hash += recv_size;
>> + curr_hash = djb2(&curr_hash, sizeof(curr_hash));
>> }
>>
>> close(fd);
>> + remote_hash = control_readulong(NULL);
>> +
>> + if (curr_hash != remote_hash) {
>> + fprintf(stderr, "Message bounds broken\n");
>> + exit(EXIT_FAILURE);
>> + }
>> }
>>
>> #define MESSAGE_TRUNC_SZ 32
>> @@ -837,6 +925,7 @@ int main(int argc, char **argv)
>> .peer_cid = VMADDR_CID_ANY,
>> };
>>
>> + srand(time(NULL));
>> init_signals();
>>
>> for (;;) {
>> --
>> 2.25.1
>
Powered by blists - more mailing lists