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-crypto" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html