Re: [PATCH 7/8] crypto: AF_ALG: add random number generator support
On 11/12/2014 06:46 PM, Stephan Mueller wrote: ... * I unconditionally use the memset after memcpy as you indicated. Once the cryptodev tree contains the memzero_explicit call, I will start picking up that function. Herbert merged it actually in this morning, so it's already part of the cryptodev tree by now. Essentially, I throught of the line you suggested. Ok, thanks. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 7/8] crypto: AF_ALG: add random number generator support
Am Mittwoch, 12. November 2014, 18:23:27 schrieb Daniel Borkmann:
Hi Daniel,
>On 11/12/2014 05:54 PM, Stephan Mueller wrote:
>> Am Mittwoch, 12. November 2014, 17:15:52 schrieb Daniel Borkmann:
>>> 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.
>>
>> Well, there is a very good reason for using the approach I have: we
>> all have done the error of forgetting the second = sign.
>>
>> In my case, the compiler will complain and we fix the error right
>> away.
>>
>> In your case, nobody is complaining but we introduced a nasty,
>> potentially hard to debug error. Thus, I very much like to keep my
>> version just to be on the safe side.
>>
>> Note, there was even a backdoor I have seen where the missing 2nd
>> equal sign introduced a privilege escalation.
>>
>> Therefore, my standard coding practice is to have a fixed value on
>> the left side and the variable on the right side of any comparison.
>
>I understand, but then please add this proposal first into ...
>
> Documentation/CodingStyle
>
>The problem is that while the rest of the kernel does not follow
>this coding style, it's also much harder to read and/or program
>this way for people not being used to. So the danger of bugs
>slipping in this way is at least equally high. Besides that, this
>argument would also only account for '==' checks.
Ok, I can change that throughout the code.
>
+ 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) .
>>
>> Right, good catch, I have to add a catch for negative error here.
>
>Hm? Don't you rather mean to say to unconditionally do something like
>...
>
> memzero_explicit(ctx->result, len);
Sorry, I was not clear:
* I need to catch a failing memcpy, but not return an error.
* I unconditionally use the memset after memcpy as you indicated. Once
the cryptodev tree contains the memzero_explicit call, I will start
picking up that function.
Essentially, I throught of the line you suggested.
Ciao
Stephan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 7/8] crypto: AF_ALG: add random number generator support
On 11/12/2014 05:54 PM, Stephan Mueller wrote:
Am Mittwoch, 12. November 2014, 17:15:52 schrieb Daniel Borkmann:
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.
Well, there is a very good reason for using the approach I have: we all have
done the error of forgetting the second = sign.
In my case, the compiler will complain and we fix the error right away.
In your case, nobody is complaining but we introduced a nasty, potentially
hard to debug error. Thus, I very much like to keep my version just to be on
the safe side.
Note, there was even a backdoor I have seen where the missing 2nd equal sign
introduced a privilege escalation.
Therefore, my standard coding practice is to have a fixed value on the left
side and the variable on the right side of any comparison.
I understand, but then please add this proposal first into ...
Documentation/CodingStyle
The problem is that while the rest of the kernel does not follow
this coding style, it's also much harder to read and/or program
this way for people not being used to. So the danger of bugs
slipping in this way is at least equally high. Besides that, this
argument would also only account for '==' checks.
+ 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) .
Right, good catch, I have to add a catch for negative error here.
Hm? Don't you rather mean to say to unconditionally do something like ...
memzero_explicit(ctx->result, len);
...
+ memset(ctx->result, 0, MAXSIZE);
memset(ctx->result, 0, sizeof(ctx->result));
Ok, if this is desired, fine with me.
Yes, please.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 7/8] crypto: AF_ALG: add random number generator support
Am Mittwoch, 12. November 2014, 17:15:52 schrieb Daniel Borkmann:
Hi Daniel,
thanks for the comments.
> 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.
Well, there is a very good reason for using the approach I have: we all have
done the error of forgetting the second = sign.
In my case, the compiler will complain and we fix the error right away.
In your case, nobody is complaining but we introduced a nasty, potentially
hard to debug error. Thus, I very much like to keep my version just to be on
the safe side.
Note, there was even a backdoor I have seen where the missing 2nd equal sign
introduced a privilege escalation.
Therefore, my standard coding practice is to have a fixed value on the left
side and the variable on the right side of any comparison.
>
> > + 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) .
Right, good catch, I have to add a catch for negative error here.
>
> > +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));
Ok, if this is desired, fine with me.
>
> > + 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...
Will do.
>
> > + 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);
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
Ciao
Stephan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at
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);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
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);
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 7/8] crypto: AF_ALG: add random number generator support
Am Mittwoch, 12. November 2014, 17:15:52 schrieb Daniel Borkmann:
Hi Daniel,
thanks for the comments.
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.
Well, there is a very good reason for using the approach I have: we all have
done the error of forgetting the second = sign.
In my case, the compiler will complain and we fix the error right away.
In your case, nobody is complaining but we introduced a nasty, potentially
hard to debug error. Thus, I very much like to keep my version just to be on
the safe side.
Note, there was even a backdoor I have seen where the missing 2nd equal sign
introduced a privilege escalation.
Therefore, my standard coding practice is to have a fixed value on the left
side and the variable on the right side of any comparison.
+ 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) .
Right, good catch, I have to add a catch for negative error here.
+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));
Ok, if this is desired, fine with me.
+ 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...
Will do.
+ 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);
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
--
Ciao
Stephan
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 7/8] crypto: AF_ALG: add random number generator support
On 11/12/2014 05:54 PM, Stephan Mueller wrote:
Am Mittwoch, 12. November 2014, 17:15:52 schrieb Daniel Borkmann:
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.
Well, there is a very good reason for using the approach I have: we all have
done the error of forgetting the second = sign.
In my case, the compiler will complain and we fix the error right away.
In your case, nobody is complaining but we introduced a nasty, potentially
hard to debug error. Thus, I very much like to keep my version just to be on
the safe side.
Note, there was even a backdoor I have seen where the missing 2nd equal sign
introduced a privilege escalation.
Therefore, my standard coding practice is to have a fixed value on the left
side and the variable on the right side of any comparison.
I understand, but then please add this proposal first into ...
Documentation/CodingStyle
The problem is that while the rest of the kernel does not follow
this coding style, it's also much harder to read and/or program
this way for people not being used to. So the danger of bugs
slipping in this way is at least equally high. Besides that, this
argument would also only account for '==' checks.
+ 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) .
Right, good catch, I have to add a catch for negative error here.
Hm? Don't you rather mean to say to unconditionally do something like ...
memzero_explicit(ctx-result, len);
...
+ memset(ctx-result, 0, MAXSIZE);
memset(ctx-result, 0, sizeof(ctx-result));
Ok, if this is desired, fine with me.
Yes, please.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 7/8] crypto: AF_ALG: add random number generator support
Am Mittwoch, 12. November 2014, 18:23:27 schrieb Daniel Borkmann:
Hi Daniel,
On 11/12/2014 05:54 PM, Stephan Mueller wrote:
Am Mittwoch, 12. November 2014, 17:15:52 schrieb Daniel Borkmann:
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.
Well, there is a very good reason for using the approach I have: we
all have done the error of forgetting the second = sign.
In my case, the compiler will complain and we fix the error right
away.
In your case, nobody is complaining but we introduced a nasty,
potentially hard to debug error. Thus, I very much like to keep my
version just to be on the safe side.
Note, there was even a backdoor I have seen where the missing 2nd
equal sign introduced a privilege escalation.
Therefore, my standard coding practice is to have a fixed value on
the left side and the variable on the right side of any comparison.
I understand, but then please add this proposal first into ...
Documentation/CodingStyle
The problem is that while the rest of the kernel does not follow
this coding style, it's also much harder to read and/or program
this way for people not being used to. So the danger of bugs
slipping in this way is at least equally high. Besides that, this
argument would also only account for '==' checks.
Ok, I can change that throughout the code.
+ 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) .
Right, good catch, I have to add a catch for negative error here.
Hm? Don't you rather mean to say to unconditionally do something like
...
memzero_explicit(ctx-result, len);
Sorry, I was not clear:
* I need to catch a failing memcpy, but not return an error.
* I unconditionally use the memset after memcpy as you indicated. Once
the cryptodev tree contains the memzero_explicit call, I will start
picking up that function.
Essentially, I throught of the line you suggested.
Ciao
Stephan
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 7/8] crypto: AF_ALG: add random number generator support
On 11/12/2014 06:46 PM, Stephan Mueller wrote: ... * I unconditionally use the memset after memcpy as you indicated. Once the cryptodev tree contains the memzero_explicit call, I will start picking up that function. Herbert merged it actually in this morning, so it's already part of the cryptodev tree by now. Essentially, I throught of the line you suggested. Ok, thanks. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
