[PATCH] avoid excessive use of socket buffer in skcipher

[PATCH] avoid excessive use of socket buffer in skcipher

[Ondrej Kozina]

Hello all,

I found this bug when I ran cryptsetup testsuite on ppc64 arch. I don't have deep
insight into networking, but it seemed to me the MAX_SGL_ENTS define doesn't have
to be tied to PAGE_SIZE. Please take this patch as base for discussion, if it's
fundamentally wrong.

The 'easy' way is to increase net.core.optmem_max, but this way we would blow up
the memory overhead for every socket in kernel. Not to mention that for user space,
without not insignificant debugging effort, it's not clear what really happened.

Kind regards
Ondrej

Ondrej Kozina (1):
  avoid excessive use of socket buffer in skcipher

 crypto/algif_skcipher.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

-- 
1.9.3

[PATCH] avoid excessive use of socket buffer in skcipher

[Ondrej Kozina]

On archs with PAGE_SIZE >= 64 KiB the function skcipher_alloc_sgl()
fails with -ENOMEM no matter what user space actually requested.
This is caused by the fact sock_kmalloc call inside the function tried
to allocate more memory than allowed by the default kernel socket buffer
size (kernel param net.core.optmem_max).

Signed-off-by: Ondrej Kozina <okozina@redhat.com>
---
 crypto/algif_skcipher.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/crypto/algif_skcipher.c b/crypto/algif_skcipher.c
index a19c027..83187f4 100644
--- a/crypto/algif_skcipher.c
+++ b/crypto/algif_skcipher.c
@@ -49,7 +49,7 @@ struct skcipher_ctx {
 	struct ablkcipher_request req;
 };
 
-#define MAX_SGL_ENTS ((PAGE_SIZE - sizeof(struct skcipher_sg_list)) / \
+#define MAX_SGL_ENTS ((4096 - sizeof(struct skcipher_sg_list)) / \
 		      sizeof(struct scatterlist) - 1)
 
 static inline int skcipher_sndbuf(struct sock *sk)
-- 
1.9.3

Re: [PATCH] avoid excessive use of socket buffer in skcipher

[Ondrej Kozina]

[-- Attachment #1: Type: text/plain, Size: 52 bytes --]

Attaching simple reproducer.

Kind regards
Ondrej



[-- Attachment #2: reproducer_ppc64.c --]
[-- Type: text/x-csrc, Size: 2889 bytes --]

#include <errno.h>
#include <malloc.h>
#include <stdint.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>

#include <linux/if_alg.h>

#include <sys/socket.h>
#include <sys/types.h>

#ifndef SOL_ALG
#define SOL_ALG 279
#endif

#define IN_SIZE 1024

#define MODE "ecb"
#define CIPHER "aes"

const char key[] = "0123456789abcdef";
const size_t key_len = sizeof(key) - 1;

static unsigned _getpagesize(void)
{
	static unsigned ps;

	if (ps)
		return ps;

	long r = sysconf(_SC_PAGESIZE);
	ps = r < 0 ? 4096 : r;

	return ps;
}

static void fail(const char *msg)
{
	fprintf(stderr, "%s. Couldn't verify the skcipher bug!\n", msg);
}

int main(void)
{
	char *in = NULL;
	int err, r = 1; /* r == 0 => the bug in skcipher */
	int opfd = -1;
	int tfmfd = -1;
	uint32_t *type;

	struct iovec iov;
	struct cmsghdr *hdr;
	struct sockaddr_alg sa = {
		.salg_family = AF_ALG,
		.salg_type = "skcipher",
	};

	char buffer[CMSG_SPACE(sizeof(*type))];

	struct msghdr msg = {
		.msg_control = buffer,
		.msg_controllen = sizeof(buffer),
		.msg_iov = &iov,
		.msg_iovlen = 1,
	};

	printf("compare folowing page_size value with net.core.optmem_max value\n");
	printf("detected system's page_size: %zu\n", _getpagesize());

	if (posix_memalign((void **)&in, _getpagesize(), IN_SIZE)) {
		perror("posix_memalign()");
		fail("memalign failed");
		goto out;
	}

	memset((void *)in, 0, IN_SIZE);

	iov.iov_base = (void*)(uintptr_t)in;
	iov.iov_len = IN_SIZE;

	hdr = CMSG_FIRSTHDR(&msg);
	if (!hdr) {
		fail("small msg_control");
		goto out;
	}

	hdr->cmsg_level = SOL_ALG;
	hdr->cmsg_type = ALG_SET_OP;
	hdr->cmsg_len = CMSG_LEN(sizeof(*type));
	type = (void*)CMSG_DATA(hdr);
	*type = ALG_OP_ENCRYPT;

	if ((tfmfd = socket(AF_ALG, SOCK_SEQPACKET, 0)) == -1) {
		perror("socket()");
		fail("socket() failed supported");
		goto out;
	}

	snprintf((char *)sa.salg_name, sizeof(sa.salg_name), "%s(%s)", MODE, CIPHER);

	if (bind(tfmfd, (struct sockaddr *)&sa, sizeof(sa)) == -1) {
		perror("bind()");
		fail("bind failed");
		goto out;
	}

	if ((opfd = accept(tfmfd, NULL, 0)) == -1) {
		perror("accept()");
		fail("accept failed");
		goto out;
	}

	/* about to test aes-ecb with key size == 128b */
	printf("calling setsockopt(), setting key with keylen==%zu\n", key_len);
	if (setsockopt(tfmfd, SOL_ALG, ALG_SET_KEY, key, key_len) == -1) {
		perror("setsockopt()");
		fail("setsockopt failed");
		goto out;
	}

	if (sendmsg(opfd, &msg, 0) != IN_SIZE) {
		err = errno;
		perror("sendmsg()");

		if (err == -ENOMEM) {
			printf("the kernel has a bug in a skcipher.\n");
			r = 0;
		}
		else
			fail("sendmsg() failed w/ different error than expected. "
			     "Can't verify the skcipher bug.\n");
	} else
		fprintf(stderr, "sendmsg() passed. No bug in skcipher.\n");

out:
	if (in)
		free((void *)in);

	if (tfmfd >= 0)
		close(tfmfd);

	if (opfd >= 0)
		close(opfd);

	return r;
}

Re: [PATCH] avoid excessive use of socket buffer in skcipher

[Ondrej Kozina]

[-- Attachment #1: Type: text/plain, Size: 160 bytes --]

On 09/01/2014 05:22 PM, Ondrej Kozina wrote:
> Attaching simple reproducer.

Sigh. Mondays... Sending fixed reproducer. Excuse my mistake.

Kind regards
Ondrej

[-- Attachment #2: reproducer_ppc64.c --]
[-- Type: text/x-csrc, Size: 2888 bytes --]

#include <errno.h>
#include <malloc.h>
#include <stdint.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>

#include <linux/if_alg.h>

#include <sys/socket.h>
#include <sys/types.h>

#ifndef SOL_ALG
#define SOL_ALG 279
#endif

#define IN_SIZE 1024

#define MODE "ecb"
#define CIPHER "aes"

const char key[] = "0123456789abcdef";
const size_t key_len = sizeof(key) - 1;

static unsigned _getpagesize(void)
{
	static unsigned ps;

	if (ps)
		return ps;

	long r = sysconf(_SC_PAGESIZE);
	ps = r < 0 ? 4096 : r;

	return ps;
}

static void fail(const char *msg)
{
	fprintf(stderr, "%s. Couldn't verify the skcipher bug!\n", msg);
}

int main(void)
{
	char *in = NULL;
	int err, r = 1; /* r == 0 => the bug in skcipher */
	int opfd = -1;
	int tfmfd = -1;
	uint32_t *type;

	struct iovec iov;
	struct cmsghdr *hdr;
	struct sockaddr_alg sa = {
		.salg_family = AF_ALG,
		.salg_type = "skcipher",
	};

	char buffer[CMSG_SPACE(sizeof(*type))];

	struct msghdr msg = {
		.msg_control = buffer,
		.msg_controllen = sizeof(buffer),
		.msg_iov = &iov,
		.msg_iovlen = 1,
	};

	printf("compare folowing page_size value with net.core.optmem_max value\n");
	printf("detected system's page_size: %zu\n", _getpagesize());

	if (posix_memalign((void **)&in, _getpagesize(), IN_SIZE)) {
		perror("posix_memalign()");
		fail("memalign failed");
		goto out;
	}

	memset((void *)in, 0, IN_SIZE);

	iov.iov_base = (void*)(uintptr_t)in;
	iov.iov_len = IN_SIZE;

	hdr = CMSG_FIRSTHDR(&msg);
	if (!hdr) {
		fail("small msg_control");
		goto out;
	}

	hdr->cmsg_level = SOL_ALG;
	hdr->cmsg_type = ALG_SET_OP;
	hdr->cmsg_len = CMSG_LEN(sizeof(*type));
	type = (void*)CMSG_DATA(hdr);
	*type = ALG_OP_ENCRYPT;

	if ((tfmfd = socket(AF_ALG, SOCK_SEQPACKET, 0)) == -1) {
		perror("socket()");
		fail("socket() failed supported");
		goto out;
	}

	snprintf((char *)sa.salg_name, sizeof(sa.salg_name), "%s(%s)", MODE, CIPHER);

	if (bind(tfmfd, (struct sockaddr *)&sa, sizeof(sa)) == -1) {
		perror("bind()");
		fail("bind failed");
		goto out;
	}

	if ((opfd = accept(tfmfd, NULL, 0)) == -1) {
		perror("accept()");
		fail("accept failed");
		goto out;
	}

	/* about to test aes-ecb with key size == 128b */
	printf("calling setsockopt(), setting key with keylen==%zu\n", key_len);
	if (setsockopt(tfmfd, SOL_ALG, ALG_SET_KEY, key, key_len) == -1) {
		perror("setsockopt()");
		fail("setsockopt failed");
		goto out;
	}

	if (sendmsg(opfd, &msg, 0) != IN_SIZE) {
		err = errno;
		perror("sendmsg()");

		if (err == ENOMEM) {
			printf("the kernel has a bug in a skcipher.\n");
			r = 0;
		}
		else
			fail("sendmsg() failed w/ different error than expected. "
			     "Can't verify the skcipher bug.\n");
	} else
		fprintf(stderr, "sendmsg() passed. No bug in skcipher.\n");

out:
	if (in)
		free((void *)in);

	if (tfmfd >= 0)
		close(tfmfd);

	if (opfd >= 0)
		close(opfd);

	return r;
}

Re: [PATCH] avoid excessive use of socket buffer in skcipher

[Herbert Xu]

On Mon, Aug 25, 2014 at 11:49:54AM +0200, Ondrej Kozina wrote:
> On archs with PAGE_SIZE >= 64 KiB the function skcipher_alloc_sgl()
> fails with -ENOMEM no matter what user space actually requested.
> This is caused by the fact sock_kmalloc call inside the function tried
> to allocate more memory than allowed by the default kernel socket buffer
> size (kernel param net.core.optmem_max).
> 
> Signed-off-by: Ondrej Kozina <okozina@redhat.com>

Patch applied.  Thanks!
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] avoid excessive use of socket buffer in skcipher

[Milan Broz]

On 09/04/2014 09:08 AM, Herbert Xu wrote:
> On Mon, Aug 25, 2014 at 11:49:54AM +0200, Ondrej Kozina wrote:
>> On archs with PAGE_SIZE >= 64 KiB the function skcipher_alloc_sgl()
>> fails with -ENOMEM no matter what user space actually requested.
>> This is caused by the fact sock_kmalloc call inside the function tried
>> to allocate more memory than allowed by the default kernel socket buffer
>> size (kernel param net.core.optmem_max).
>>
>> Signed-off-by: Ondrej Kozina <okozina@redhat.com>
> 
> Patch applied.  Thanks!

Please could you send this also to stable tree?
Upstream commit now is e2cffb5f493a8b431dc87124388ea59b79f0bccb

I think it is the problem in all kernels using large page size and skcipher api.

Thanks,
Milan

[PATCH] avoid excessive use of socket buffer in skcipher

[Ondrej Kozina]

On archs with PAGE_SIZE >= 64 KiB the function skcipher_alloc_sgl()
fails with -ENOMEM no matter what user space actually requested.
This is caused by the fact sock_kmalloc call inside the function tried
to allocate more memory than allowed by the default kernel socket buffer
size (kernel param net.core.optmem_max).

Signed-off-by: Ondrej Kozina <okozina@redhat.com>
---
 crypto/algif_skcipher.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/crypto/algif_skcipher.c b/crypto/algif_skcipher.c
index a19c027..83187f4 100644
--- a/crypto/algif_skcipher.c
+++ b/crypto/algif_skcipher.c
@@ -49,7 +49,7 @@ struct skcipher_ctx {
 	struct ablkcipher_request req;
 };
 
-#define MAX_SGL_ENTS ((PAGE_SIZE - sizeof(struct skcipher_sg_list)) / \
+#define MAX_SGL_ENTS ((4096 - sizeof(struct skcipher_sg_list)) / \
 		      sizeof(struct scatterlist) - 1)
 
 static inline int skcipher_sndbuf(struct sock *sk)
-- 
1.9.3