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 PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Sat, 19 Jul 2014 08:20:11 +0200 From: Hannes Frederic Sowa <hannes@...essinduktion.org> To: Theodore Ts'o <tytso@....edu> Cc: linux-kernel@...r.kernel.org, davej@...hat.com, price@....edu Subject: Re: [PATCH] random: check for increase of entropy_count because of signed conversion On Sa, 2014-07-19 at 01:42 -0400, Theodore Ts'o wrote: > On Sat, Jul 19, 2014 at 01:35:48AM +0200, Hannes Frederic Sowa wrote: > > > + nfrac = ibytes << (ENTROPY_SHIFT + 3); > > > + if (entropy_count < 0) { > > > > Minor nit: maybe also add an unlikely() here? > > Yep, done. > > > > + if ((unsigned) entropy_count > nfrac) > > > > (unsigned) -> (size_t) > > > > size_t could also be (unsigned long) so the plain (unsigned) is > > misleading. > > Good point, done. > > > (Maybe I wouldn't have done the cast at all, as we compile the kernel > > with -Wno-sign-compare and we have the < 0 check right above, but I > > don't have a strong opinion on that.) > > I also wanted to shut up other static code checkers like Coverity. :-) > > > > + nbytes = min_t(size_t, nbytes, INT_MAX >> ENTROPY_SHIFT); > > > > Hmm, not sure, nfracs unit is 1/8 bits, so don't we have to limit nbytes > > to INT_MAX >> (ENTROPY_SHIFT + 3) here? > > Good catch, done. > > > And if we want to be even more correct here, we could switch from > > INT_MAX to SIZE_MAX, as we do all nfrac calculations in the size_t > > domain. > > The main reason why I used INT_MAX was as a further safety check to > protect the: > > entropy_count -= nfrac; > > calculation, since nfrac is size_t and entropy_count is int. > > In fact I think this online change ("nbytes = min_t(size_t, nbytes, > INT_MAX >> (ENTROPY_SHIFT + 3));") would have been enough to fix the > problem all by itself, but the other changes results in code which is > cleaner and easier to understand, and I'm a firm believer in multiple > layers of protection. :-) I see and can agree here. :) I think the patch is good to go. Thanks you, Hannes -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@...r.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists