| Message ID | 657897b90b8344eeab10d7a0f604988d@aptaiexm02f.ap.qualcomm.com (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Kalle Valo |
| Headers | show |
Am Montag, 8. August 2016, 02:03:36 CEST schrieb Pan, Miaoqing: Hi Miaoqing, > The entropy was evaluated by crypto expert, the analysis report show the > ADC with at least 10bits and up to 22 bits of min-entropy for a 32 bits > value, we conservatively assume the min-entropy is 10 bits out of 32 bits, > so that's why set entropy quality to 320/1024 = 10/32. Also we have > explained in the commit message why can't use the HW RNG framework. Where is the description of the RNG, where is the test implementation? > > Otherwise, your patch will cause high CPU load, as continuously read ADC > data if entropy bits under write_wakeup_threshold. The issue is that although you may have analyzed it, others are unable to measure the quality of the RNG and assess the design as well as the implementation of the RNG. This RNG is the only implementation of a hardware RNG that per default and without being able to change it at runtime injects data into the input_pool where the noise source cannot be audited. Note, even other respected RNG noise sources like the Intel RDRAND will not feed into / dev/random per default in a way that dominates all other noise sources. I would like to be able to deactivate that noise source to the extent that it does not cause /dev/random to unblock. The reason is that your noise source starts to dominate all other noise sources. If you think that this patch is a challenge because your driver starts to spin, please help and offer another solution. > > -- > Miaoqing > > -----Original Message----- > From: Stephan Mueller [mailto:smueller@chronox.de] > Sent: Sunday, August 07, 2016 5:36 PM > To: Ted Tso <tytso@mit.edu> > Cc: herbert@gondor.apana.org.au; linux-kernel@vger.kernel.org; > linux-crypto@vger.kernel.org; ath9k-devel <ath9k-devel@qca.qualcomm.com>; > linux-wireless@vger.kernel.org; ath9k-devel@lists.ath9k.org; Kalle Valo > <kvalo@codeaurora.org>; Jason Cooper <jason@lakedaemon.net> Subject: [PATCH > v2] RANDOM: ATH9K RNG delivers zero bits of entropy > > The ATH9K driver implements an RNG which is completely bypassing the > standard Linux HW generator logic. > > The RNG may or may not deliver entropy. Considering the conservative > approach in treating entropy with respect to non-auditable sources, this > patch changes the delivered entropy value to zero. The RNG still feeds data > into the input_pool but it is assumed to have no entropy. > > When the ATH9K RNG changes to use the HW RNG framework, it may re-enable the > entropy estimation considering that a user can change that value at boot > and runtime. > > Reviewed-by: Jason Cooper <jason@lakedaemon.net> > Signed-off-by: Stephan Mueller <smueller@chronox.de> > --- > drivers/net/wireless/ath/ath9k/rng.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > diff --git a/drivers/net/wireless/ath/ath9k/rng.c > b/drivers/net/wireless/ath/ath9k/rng.c index d38e50f..1ed8338 100644 > --- a/drivers/net/wireless/ath/ath9k/rng.c > +++ b/drivers/net/wireless/ath/ath9k/rng.c > @@ -22,7 +22,6 @@ > #include "ar9003_phy.h" > > #define ATH9K_RNG_BUF_SIZE 320 > -#define ATH9K_RNG_ENTROPY(x) (((x) * 8 * 320) >> 10) /* quality: 320/1024 > */ > > static int ath9k_rng_data_read(struct ath_softc *sc, u32 *buf, u32 > buf_size) { @@ -92,8 +91,7 @@ static int ath9k_rng_kthread(void *data) > fail_stats = 0; > > /* sleep until entropy bits under write_wakeup_threshold */ > - add_hwgenerator_randomness((void *)rng_buf, bytes_read, > - ATH9K_RNG_ENTROPY(bytes_read)); > + add_hwgenerator_randomness((void *)rng_buf, bytes_read, 0); > } > > kfree(rng_buf); > -- > 2.7.4 Ciao Stephan -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Stephan, Miaoqing Pan, On Mon, Aug 08, 2016 at 08:41:36AM +0200, Stephan Mueller wrote: > Am Montag, 8. August 2016, 02:03:36 CEST schrieb Pan, Miaoqing: > > The entropy was evaluated by crypto expert, the analysis report show the > > ADC with at least 10bits and up to 22 bits of min-entropy for a 32 bits > > value, we conservatively assume the min-entropy is 10 bits out of 32 bits, > > so that's why set entropy quality to 320/1024 = 10/32. Ok, so the relevant commit is: ed14dc0af7cce ath9k: feeding entropy in kernel from ADC capture Which refers to a previous commit: 6301566e0b2d ath9k: export HW random number generator > > Also we have explained in the commit message why can't use the HW > > RNG framework. From ed14dc0af7cce: """ Since ADC was not designed to be a dedicated HW RNG, we do not want to bind it to /dev/hwrng framework directly. """ > Where is the description of the RNG, where is the test implementation? > > > > Otherwise, your patch will cause high CPU load, as continuously read ADC > > data if entropy bits under write_wakeup_threshold. > > The issue is that although you may have analyzed it, others are unable to > measure the quality of the RNG and assess the design as well as the > implementation of the RNG. This RNG is the only implementation of a hardware > RNG that per default and without being able to change it at runtime injects > data into the input_pool where the noise source cannot be audited. Note, even > other respected RNG noise sources like the Intel RDRAND will not feed into / > dev/random per default in a way that dominates all other noise sources. > > I would like to be able to deactivate that noise source to the extent that it > does not cause /dev/random to unblock. The reason is that your noise source > starts to dominate all other noise sources. I think the short-term problem here is the config logic: config ATH9K_HWRNG bool "Random number generator support" depends on ATH9K && (HW_RANDOM = y || HW_RANDOM = ATH9K) default y If you have *any* hwrngs you want to use and you have an ath9k card (HW_RANDOM = y and ATH9K != n), you get the behavior Stephan is pointing out. Short term, we should just default no here. > If you think that this patch is a challenge because your driver starts to > spin, please help and offer another solution. Well, I don't buy the reasoning listed above for not using the hwrng framework. Interrupt timings were never designed to be a source of entropy either. We need to grab it where ever we can find it, especially on embedded systems. Documentation/hw_random.txt even says: """ This data is NOT CHECKED by any fitness tests, and could potentially be bogus (if the hardware is faulty or has been tampered with). """ I really don't think there's a problem with adding these sorts of sources under char/hw_random/. I think the only thing we would be concerned about, other than the already addressed entropy estimation, would be constraining the data rate. Is ath9k the only wireless card that exposes ADC registers? What about sound cards? thx, Jason. -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Stephan, On Mon, Aug 08, 2016 at 05:29:30PM +0000, Jason Cooper wrote: > On Mon, Aug 08, 2016 at 08:41:36AM +0200, Stephan Mueller wrote: ... > > If you think that this patch is a challenge because your driver starts to > > spin, please help and offer another solution. > > Well, I don't buy the reasoning listed above for not using the hwrng > framework. Interrupt timings were never designed to be a source of entropy > either. We need to grab it where ever we can find it, especially on > embedded systems. Documentation/hw_random.txt even says: > > """ > This data is NOT CHECKED by any fitness tests, and could potentially be > bogus (if the hardware is faulty or has been tampered with). > """ > > I really don't think there's a problem with adding these sorts of > sources under char/hw_random/. I think the only thing we would be > concerned about, other than the already addressed entropy estimation, > would be constraining the data rate. Further research yields char/hw_random/timeriomem-rng.c It could use an update to ->read() vice data_{present,read}(), but it's functionally exactly what the ath9k rng is doing. :) thx, Jason. -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Jason, Stephan, Agree with Jason's point, also understand Stephan's concern. The date rate can be roughly estimated by 'cat /dev/random |rngtest -c 1000', the average speed is 1111.294Kibits/s. I will sent the patch to disable ath9k RNG by default. Thanks, Miaoqing -----Original Message----- From: Jason Cooper [mailto:jason@lakedaemon.net] Sent: Tuesday, August 09, 2016 1:30 AM To: Stephan Mueller <smueller@chronox.de> Cc: Pan, Miaoqing <miaoqing@qti.qualcomm.com>; Ted Tso <tytso@mit.edu>; Sepehrdad, Pouyan <pouyans@qti.qualcomm.com>; herbert@gondor.apana.org.au; linux-kernel@vger.kernel.org; linux-crypto@vger.kernel.org; ath9k-devel <ath9k-devel@qca.qualcomm.com>; linux-wireless@vger.kernel.org; ath9k-devel@lists.ath9k.org; Kalle Valo <kvalo@codeaurora.org> Subject: Re: [PATCH v2] RANDOM: ATH9K RNG delivers zero bits of entropy Hi Stephan, Miaoqing Pan, On Mon, Aug 08, 2016 at 08:41:36AM +0200, Stephan Mueller wrote: > Am Montag, 8. August 2016, 02:03:36 CEST schrieb Pan, Miaoqing: > > The entropy was evaluated by crypto expert, the analysis report > > show the ADC with at least 10bits and up to 22 bits of min-entropy > > for a 32 bits value, we conservatively assume the min-entropy is 10 > > bits out of 32 bits, so that's why set entropy quality to 320/1024 = 10/32. Ok, so the relevant commit is: ed14dc0af7cce ath9k: feeding entropy in kernel from ADC capture Which refers to a previous commit: 6301566e0b2d ath9k: export HW random number generator > > Also we have explained in the commit message why can't use the HW > > RNG framework. From ed14dc0af7cce: """ Since ADC was not designed to be a dedicated HW RNG, we do not want to bind it to /dev/hwrng framework directly. """ > Where is the description of the RNG, where is the test implementation? > > > > Otherwise, your patch will cause high CPU load, as continuously > > read ADC data if entropy bits under write_wakeup_threshold. > > The issue is that although you may have analyzed it, others are unable > to measure the quality of the RNG and assess the design as well as the > implementation of the RNG. This RNG is the only implementation of a > hardware RNG that per default and without being able to change it at > runtime injects data into the input_pool where the noise source cannot > be audited. Note, even other respected RNG noise sources like the > Intel RDRAND will not feed into / dev/random per default in a way that dominates all other noise sources. > > I would like to be able to deactivate that noise source to the extent > that it does not cause /dev/random to unblock. The reason is that your > noise source starts to dominate all other noise sources. I think the short-term problem here is the config logic: config ATH9K_HWRNG bool "Random number generator support" depends on ATH9K && (HW_RANDOM = y || HW_RANDOM = ATH9K) default y If you have *any* hwrngs you want to use and you have an ath9k card (HW_RANDOM = y and ATH9K != n), you get the behavior Stephan is pointing out. Short term, we should just default no here. > If you think that this patch is a challenge because your driver starts > to spin, please help and offer another solution. Well, I don't buy the reasoning listed above for not using the hwrng framework. Interrupt timings were never designed to be a source of entropy either. We need to grab it where ever we can find it, especially on embedded systems. Documentation/hw_random.txt even says: """ This data is NOT CHECKED by any fitness tests, and could potentially be bogus (if the hardware is faulty or has been tampered with). """ I really don't think there's a problem with adding these sorts of sources under char/hw_random/. I think the only thing we would be concerned about, other than the already addressed entropy estimation, would be constraining the data rate. Is ath9k the only wireless card that exposes ADC registers? What about sound cards? thx, Jason. -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Aug 09, 2016 at 06:30:03AM +0000, Pan, Miaoqing wrote: > Agree with Jason's point, also understand Stephan's concern. The > date rate can be roughly estimated by 'cat /dev/random |rngtest -c > 1000', the average speed is 1111.294Kibits/s. I will sent the patch > to disable ath9k RNG by default. If the ATH9K is generating some random amount of data, but you don't know how random, and you're gathering it opportunistically --- for example, suppose a wireless driver gets the radio's signal strength through the normal course of its operation, and while there might not be that much randomness for someone who can observe the exact details of how the phone is positioned in the room --- but for which the analyst sitting in Fort Meade won't know whether or not the phone is on the desk, or in a knapsack under the table, the right interface to use is: void add_device_randomness(const void *buf, unsigned int size); This won't increase the entropy count, but if you have the bit of potential randomness "for free", you might as well contribute it to the entropy pool. If it turns out that the chip was manufactured in China, and the MSS has backdoored it out the wazoo, it won't do any harm --- where as using the hwrng framework would be disastrous. Cheerfs, - Ted -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Ted, On Tue, Aug 09, 2016 at 07:56:22AM -0400, Theodore Ts'o wrote: > On Tue, Aug 09, 2016 at 06:30:03AM +0000, Pan, Miaoqing wrote: > > Agree with Jason's point, also understand Stephan's concern. The > > date rate can be roughly estimated by 'cat /dev/random |rngtest -c > > 1000', the average speed is 1111.294Kibits/s. I will sent the patch > > to disable ath9k RNG by default. > > If the ATH9K is generating some random amount of data, but you don't > know how random, and you're gathering it opportunistically --- for > example, suppose a wireless driver gets the radio's signal strength > through the normal course of its operation, and while there might not > be that much randomness for someone who can observe the exact details > of how the phone is positioned in the room --- but for which the > analyst sitting in Fort Meade won't know whether or not the phone is > on the desk, or in a knapsack under the table, the right interface to > use is: > > void add_device_randomness(const void *buf, unsigned int size); > > This won't increase the entropy count, but if you have the bit of > potential randomness "for free", you might as well contribute it to > the entropy pool. If it turns out that the chip was manufactured in > China, and the MSS has backdoored it out the wazoo, it won't do any > harm --- where as using the hwrng framework would be disastrous. Ok, I get that. However, we have Documentation/hw_random.txt saying: This data is NOT CHECKED by any fitness tests, and could potentially be bogus (if the hardware is faulty or has been tampered with). Data is only output if the hardware "has-data" flag is set, but nevertheless a security-conscious person would run fitness tests on the data before assuming it is truly random. Which I would read as "Don't assume 1 bit read from /dev/hwrng equals 1 bit of entropy." Which runs counter to Stephan's reading of the rngd code. And then we have drivers like timeriomem-rng.c that appear to be spitting out the raw 32bit register value of $SOC's timer. Thankfully, most hw_random drivers don't set the quality. So unless the user sets the default_quality param, it's zero. iiuc, Ted, you're saying using the hw_random framework would be disasterous because despite most drivers having a default quality of 0, rngd assumes 1 bit of entropy for every bit read? thx, Jason. -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Aug 09, 2016 at 02:04:44PM +0000, Jason Cooper wrote: > > iiuc, Ted, you're saying using the hw_random framework would be > disasterous because despite most drivers having a default quality of 0, > rngd assumes 1 bit of entropy for every bit read? Sorry, what I was trying to say (but failed) was that bypassing the hwrng framework and injecting entropy directly the entropy pool was disatrous. > Thankfully, most hw_random drivers don't set the quality. So unless the > user sets the default_quality param, it's zero. The fact that this is "most" and not "all" does scare me a little. As far as I'm concerned *all* hw_random drivers should set quality to zero, since it should be up to the system administrator. Perhaps the one exception is virtio_rng, since if you don't trust the hypvervisor, the security of the VM is hopeless. That being said, I have seen configurations of KVM which use: -object rng-random,filename=/dev/urandom,id=rng0 \ -device virtio-rng-pci,rng=rng0 Which is somewhat non-ideal. (Try running od -x /dev/random on such a guest system....) - Ted -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hey Ted, On Wed, Aug 10, 2016 at 07:44:25PM -0400, Theodore Ts'o wrote: > On Tue, Aug 09, 2016 at 02:04:44PM +0000, Jason Cooper wrote: > > iiuc, Ted, you're saying using the hw_random framework would be > > disasterous because despite most drivers having a default quality of 0, > > rngd assumes 1 bit of entropy for every bit read? > > Sorry, what I was trying to say (but failed) was that bypassing the > hwrng framework and injecting entropy directly the entropy pool was > disatrous. Ok, whew. :) > > Thankfully, most hw_random drivers don't set the quality. So unless the > > user sets the default_quality param, it's zero. > > The fact that this is "most" and not "all" does scare me a little. My recent grep showed that only virtio-rng set it to a non-zero value. > As far as I'm concerned *all* hw_random drivers should set quality to > zero, since it should be up to the system administrator. Agreed. Gathering conversation about this from a few related threads, I have one concern. Apparently there is some confusion in userspace consumers of /dev/hwrng data as to the quality of it. Specifically, rngd (spotted by Stephan Mueller) appears to assume 1bit of entropy per 1 bit read. :-/ So, while moving ath9k-rng to the hwrng framework makes complete sense internally, it's not so good for existing userspace assumptions. I'd think that timeriomem-rng falls in this same category. In light of this, do you think it's worth the effort (I'm volunteering) to create a subcategory of hwrng drivers that are 'environemntal' rngs? They can contribute to the kernel entropy pools, but not to /dev/hwrng. thx, Jason. -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
"Pan, Miaoqing" <miaoqing@qti.qualcomm.com> writes: > The entropy was evaluated by crypto expert, the analysis report show > the ADC with at least 10bits and up to 22 bits of min-entropy for a 32 > bits value, we conservatively assume the min-entropy is 10 bits out of > 32 bits, so that's why set entropy quality to 320/1024 = 10/32. Also > we have explained in the commit message why can't use the HW RNG > framework. > > Otherwise, your patch will cause high CPU load, as continuously read > ADC data if entropy bits under write_wakeup_threshold. Please don't top post, it breaks patchwork which is extremely annoying for me: https://patchwork.kernel.org/patch/9266265/ https://patchwork.kernel.org/patch/9266617/
diff --git a/drivers/net/wireless/ath/ath9k/rng.c b/drivers/net/wireless/ath/ath9k/rng.c index d38e50f..1ed8338 100644 --- a/drivers/net/wireless/ath/ath9k/rng.c +++ b/drivers/net/wireless/ath/ath9k/rng.c @@ -22,7 +22,6 @@ #include "ar9003_phy.h" #define ATH9K_RNG_BUF_SIZE 320 -#define ATH9K_RNG_ENTROPY(x) (((x) * 8 * 320) >> 10) /* quality: 320/1024 */ static int ath9k_rng_data_read(struct ath_softc *sc, u32 *buf, u32 buf_size) { @@ -92,8 +91,7 @@ static int ath9k_rng_kthread(void *data) fail_stats = 0;