Messages in this thread | | | Date | Wed, 12 Nov 2014 17:15:52 +0100 | From | Daniel Borkmann <> | Subject | Re: [PATCH 7/8] crypto: AF_ALG: add random number generator support |
| |
On 11/12/2014 08:05 AM, Stephan Mueller wrote: > This patch adds the random number generator support for AF_ALG. > > A random number generator's purpose is to generate data without > requiring the caller to provide any data. Therefore, the AF_ALG > interface handler for RNGs only implements a callback handler for > recvmsg. ... > +static int rng_recvmsg(struct kiocb *unused, struct socket *sock, > + struct msghdr *msg, size_t len, int flags) > +{ > + struct sock *sk = sock->sk; > + struct alg_sock *ask = alg_sk(sk); > + struct rng_ctx *ctx = ask->private; > + int err = -EFAULT; > + > + if (0 == len)
if (len == 0) ...
[And also other places.]
We don't use Yoda condition style in the kernel.
> + return 0; > + if (MAXSIZE < len) > + len = MAXSIZE; > + > + lock_sock(sk); > + len = crypto_rng_get_bytes(ctx->drng, ctx->result, len); > + if (0 > len) > + goto unlock; > + > + err = memcpy_toiovec(msg->msg_iov, ctx->result, len); > + memset(ctx->result, 0, err); > +
This looks buggy.
If copy_to_user() fails from within memcpy_toiovec(), we call memset() with a negative return value which is interpreted as size_t and thus causes a buffer overflow writing beyond ctx->result, no?
If it succeeds, we call memset(ctx->result, 0, 0) .....
> +unlock: > + release_sock(sk); > + > + return err ? err : len; > +} > + > +static struct proto_ops algif_rng_ops = { > + .family = PF_ALG, > + > + .connect = sock_no_connect, > + .socketpair = sock_no_socketpair, > + .getname = sock_no_getname, > + .ioctl = sock_no_ioctl, > + .listen = sock_no_listen, > + .shutdown = sock_no_shutdown, > + .getsockopt = sock_no_getsockopt, > + .mmap = sock_no_mmap, > + .bind = sock_no_bind, > + .accept = sock_no_accept, > + .setsockopt = sock_no_setsockopt, > + .poll = sock_no_poll, > + .sendmsg = sock_no_sendmsg, > + .sendpage = sock_no_sendpage, > + > + .release = af_alg_release, > + .recvmsg = rng_recvmsg, > +}; > + > +static void *rng_bind(const char *name, u32 type, u32 mask) > +{ > + return crypto_alloc_rng(name, type, mask); > +} > + > +static void rng_release(void *private) > +{ > + crypto_free_rng(private); > +} > + > +static void rng_sock_destruct(struct sock *sk) > +{ > + struct alg_sock *ask = alg_sk(sk); > + struct rng_ctx *ctx = ask->private; > + > + memset(ctx->result, 0, MAXSIZE);
memset(ctx->result, 0, sizeof(ctx->result));
> + sock_kfree_s(sk, ctx, ctx->len); > + af_alg_release_parent(sk); > +} > + > +static int rng_accept_parent(void *private, struct sock *sk) > +{ > + struct rng_ctx *ctx; > + struct alg_sock *ask = alg_sk(sk); > + unsigned int len = sizeof(*ctx); > + int seedsize = crypto_rng_seedsize(private); > + int ret = -ENOMEM; > + > + ctx = sock_kmalloc(sk, len, GFP_KERNEL); > + if (!ctx) > + return -ENOMEM; > + memset(ctx->result, 0, MAXSIZE);
Ditto...
> + ctx->len = len; > + > + if (seedsize) { > + u8 *buf = kmalloc(seedsize, GFP_KERNEL); > + if (!buf) > + goto err; > + get_random_bytes(buf, seedsize); > + ret = crypto_rng_reset(private, buf, len); > + kzfree(buf);
| |