[v2] crypto: sun4i-ss: support the Security System PRNG

Commit Message

Corentin Labbe Dec. 13, 2016, 2:10 p.m. UTC

On Thu, Dec 08, 2016 at 05:06:18PM +0800, Herbert Xu wrote:
> On Wed, Dec 07, 2016 at 01:51:27PM +0100, Corentin Labbe wrote:
> > 
> > So I must expose it as a crypto_rng ?
> 
> If it is to be exposed at all then algif_rng would be the best
> place.
> 
> > Could you explain why PRNG must not be used as hw_random ?
> 
> The hwrng interface was always meant to be an interface for real
> hardware random number generators.  People rely on that so we
> should not provide bogus entropy sources through this interface.
> 
> Cheers,

I have found two solutions:

The simplier is to add an attribute isprng to hwrng like that:

Regards
Corentin Labbe

Comments

PrasannaKumar Muralidharan Dec. 13, 2016, 3:23 p.m. UTC | #1

> What do you think about those two solutions ?

I prefer the second solution's idea of using two files (/dev/hwrng and
/dev/hwprng). Upon having a quick glance it looks like (based on
current_rng == prng check) that your current implementation allows
only one rng device to be in use at a time. It would be better to have
both usable at the same time. So applications that need pseudo random
data at high speed can use /dev/prng while applications that require
true random number can use /dev/rng. Please feel free to correct if my
understanding of the code is incorrect. Along with this change I think
changing the algif_rng to use this code if this solution is going to
be used.

Regards,
PrasannaKumar

Corentin Labbe Dec. 13, 2016, 3:33 p.m. UTC | #2

On Tue, Dec 13, 2016 at 08:53:54PM +0530, PrasannaKumar Muralidharan wrote:
> > What do you think about those two solutions ?
> 
> I prefer the second solution's idea of using two files (/dev/hwrng and
> /dev/hwprng). Upon having a quick glance it looks like (based on
> current_rng == prng check) that your current implementation allows
> only one rng device to be in use at a time. It would be better to have
> both usable at the same time. So applications that need pseudo random
> data at high speed can use /dev/prng while applications that require
> true random number can use /dev/rng. Please feel free to correct if my
> understanding of the code is incorrect. Along with this change I think
> changing the algif_rng to use this code if this solution is going to
> be used.
> 

No, there could be both device at the same time.

Herbert Xu Dec. 14, 2016, 5:05 a.m. UTC | #3

On Tue, Dec 13, 2016 at 03:10:59PM +0100, Corentin Labbe wrote:
>
> I have found two solutions:

No we already have algif_rng so let's not confuse things even
further by making hwrng take PRNGs.

Cheers,

Corentin Labbe Dec. 14, 2016, 6:03 a.m. UTC | #4

On Wed, Dec 14, 2016 at 01:05:51PM +0800, Herbert Xu wrote:
> On Tue, Dec 13, 2016 at 03:10:59PM +0100, Corentin Labbe wrote:
> >
> > I have found two solutions:
> 
> No we already have algif_rng so let's not confuse things even
> further by making hwrng take PRNGs.
> 

But algif_rng is not accessible from user space without any coding.
So no easy "random" data with some cat /dev/xxxx.

Clearly users of the 3 already intree hw_random PRNG will see that like a regresion.

Regards
Corentin Labbe

PrasannaKumar Muralidharan Dec. 14, 2016, 7:17 p.m. UTC | #5

>> I have found two solutions:
>
> No we already have algif_rng so let's not confuse things even
> further by making hwrng take PRNGs.

Even if both the solutions could not be adopted I think there must be
a way for applications to use similar API to get true rng or prng.
Given the case that no user complained about prng data when using
/dev/hwrng is it safe to assume that the random data generated is
acceptable for users? If so, the drivers can be left without any
modification.

Should there be a mandate that driver will be accepted only when it
passes 'rngtest'. This will make sure that prng drivers won't get
added in future.

Regards,
PrasannaKumar

Herbert Xu Dec. 15, 2016, 6:09 a.m. UTC | #6

On Thu, Dec 15, 2016 at 12:47:16AM +0530, PrasannaKumar Muralidharan wrote:
> Should there be a mandate that driver will be accepted only when it
> passes 'rngtest'. This will make sure that prng drivers won't get
> added in future.

You cannot use software to distinguish between a PRNG and an HRNG.
We can only rely on the veracity of the documentation.

Cheers,

Patch

--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -150,7 +150,8 @@  static int hwrng_init(struct hwrng *rng)
        reinit_completion(&rng->cleanup_done);
 
 skip_init:
-       add_early_randomness(rng);
+       if (!rng->isprng)
+               add_early_randomness(rng);
 
        current_quality = rng->quality ? : default_quality;
        if (current_quality > 1024)
@@ -158,7 +159,7 @@  static int hwrng_init(struct hwrng *rng)
 
        if (current_quality == 0 && hwrng_fill)
                kthread_stop(hwrng_fill);
-       if (current_quality > 0 && !hwrng_fill)
+       if (current_quality > 0 && !hwrng_fill && !rng->isprng)
                start_khwrngd();
 
        return 0;
@@ -439,7 +440,7 @@  int hwrng_register(struct hwrng *rng)
        }
        list_add_tail(&rng->list, &rng_list);
 
-       if (old_rng && !rng->init) {
+       if (old_rng && !rng->init && !rng->isprng) {
                /*
                 * Use a new device's input to add some randomness to
                 * the system.  If this rng device isn't going to be
diff --git a/include/linux/hw_random.h b/include/linux/hw_random.h
index 34a0dc1..5a5b8dc 100644
--- a/include/linux/hw_random.h
+++ b/include/linux/hw_random.h
@@ -50,6 +50,7 @@  struct hwrng {
        struct list_head list;
        struct kref ref;
        struct completion cleanup_done;
+       bool isprng;
 };


With that, we still could register prng, but they dont provide any entropy.
An optional Kconfig/"module parameter" could still be added for people still wanting this old behavour.


The other solution is to "duplicate" /dev/hwrng to /dev/hwprng like that:
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -24,8 +24,11 @@ 
 #include <linux/uaccess.h>
 
 #define RNG_MODULE_NAME		"hw_random"
+#define PRNG_MODULE_NAME	"hw_prng"
+#define HWPRNG_MINOR	185 /* not official */
 
 static struct hwrng *current_rng;
+static struct hwrng *current_prng;
 static struct task_struct *hwrng_fill;
 static LIST_HEAD(rng_list);
 /* Protects rng_list and current_rng */
@@ -44,7 +47,7 @@  module_param(default_quality, ushort, 0644);
 MODULE_PARM_DESC(default_quality,
 		 "default entropy content of hwrng per mill");
 
-static void drop_current_rng(void);
+static void drop_current_rng(bool prng);
 static int hwrng_init(struct hwrng *rng);
 static void start_khwrngd(void);
 
@@ -56,6 +59,14 @@  static size_t rng_buffer_size(void)
 	return SMP_CACHE_BYTES < 32 ? 32 : SMP_CACHE_BYTES;
 }
 
+static bool is_current_xrng(struct hwrng *rng)
+{
+	if (rng->isprng)
+		return (rng == current_prng);
+	else
+		return (rng == current_rng);
+}
+
 static void add_early_randomness(struct hwrng *rng)
 {
 	int bytes_read;
@@ -88,32 +99,46 @@  static int set_current_rng(struct hwrng *rng)
 	if (err)
 		return err;
 
-	drop_current_rng();
-	current_rng = rng;
+	drop_current_rng(rng->isprng);
+	if (rng->isprng)
+		current_prng = rng;
+	else
+		current_rng = rng;
 
 	return 0;
 }
 
-static void drop_current_rng(void)
+static void drop_current_rng(bool prng)
 {
 	BUG_ON(!mutex_is_locked(&rng_mutex));
-	if (!current_rng)
-		return;
-
-	/* decrease last reference for triggering the cleanup */
-	kref_put(&current_rng->ref, cleanup_rng);
-	current_rng = NULL;
+	if (prng) {
+		if (!current_prng)
+			return;
+		/* decrease last reference for triggering the cleanup */
+		kref_put(&current_prng->ref, cleanup_rng);
+		current_prng = NULL;
+	} else {
+		if (!current_rng)
+			return;
+		/* decrease last reference for triggering the cleanup */
+		kref_put(&current_rng->ref, cleanup_rng);
+		current_rng = NULL;
+	}
 }
 
 /* Returns ERR_PTR(), NULL or refcounted hwrng */
-static struct hwrng *get_current_rng(void)
+static struct hwrng *get_current_rng(bool prng)
 {
 	struct hwrng *rng;
 
 	if (mutex_lock_interruptible(&rng_mutex))
 		return ERR_PTR(-ERESTARTSYS);
 
-	rng = current_rng;
+	if (prng)
+		rng = current_prng;
+	else
+		rng = current_rng;
+
 	if (rng)
 		kref_get(&rng->ref);
 
@@ -193,8 +218,8 @@  static inline int rng_get_data(struct hwrng *rng, u8 *buffer, size_t size,
 	return 0;
 }
 
-static ssize_t rng_dev_read(struct file *filp, char __user *buf,
-			    size_t size, loff_t *offp)
+static ssize_t genrng_dev_read(struct file *filp, char __user *buf,
+			    size_t size, loff_t *offp, bool prng)
 {
 	ssize_t ret = 0;
 	int err = 0;
@@ -202,7 +227,7 @@  static ssize_t rng_dev_read(struct file *filp, char __user *buf,
 	struct hwrng *rng;
 
 	while (size) {
-		rng = get_current_rng();
+		rng = get_current_rng(prng);
 		if (IS_ERR(rng)) {
 			err = PTR_ERR(rng);
 			goto out;
@@ -270,6 +295,18 @@  static ssize_t rng_dev_read(struct file *filp, char __user *buf,
 	goto out;
 }
 
+static ssize_t rng_dev_read(struct file *filp, char __user *buf,
+			    size_t size, loff_t *offp)
+{
+	return genrng_dev_read(filp, buf, size, offp, false);
+}
+
+static ssize_t prng_dev_read(struct file *filp, char __user *buf,
+			    size_t size, loff_t *offp)
+{
+	return genrng_dev_read(filp, buf, size, offp, true);
+}
+
 static const struct file_operations rng_chrdev_ops = {
 	.owner		= THIS_MODULE,
 	.open		= rng_dev_open,
@@ -278,6 +315,7 @@  static const struct file_operations rng_chrdev_ops = {
 };
 
 static const struct attribute_group *rng_dev_groups[];
+static const struct attribute_group *prng_dev_groups[];
 
 static struct miscdevice rng_miscdev = {
 	.minor		= HWRNG_MINOR,
@@ -287,9 +325,24 @@  static struct miscdevice rng_miscdev = {
 	.groups		= rng_dev_groups,
 };
 
-static ssize_t hwrng_attr_current_store(struct device *dev,
-					struct device_attribute *attr,
-					const char *buf, size_t len)
+static const struct file_operations prng_chrdev_ops = {
+	.owner		= THIS_MODULE,
+	.open		= rng_dev_open,
+	.read		= prng_dev_read,
+	.llseek		= noop_llseek,
+};
+
+static struct miscdevice prng_miscdev = {
+	.minor		= HWPRNG_MINOR,
+	.name		= PRNG_MODULE_NAME,
+	.nodename	= "hwprng",
+	.fops		= &prng_chrdev_ops,
+	.groups		= prng_dev_groups,
+};
+
+static ssize_t genrng_attr_current_store(struct device *dev,
+					 struct device_attribute *attr,
+					 const char *buf, size_t len, bool prng)
 {
 	int err;
 	struct hwrng *rng;
@@ -301,8 +354,13 @@  static ssize_t hwrng_attr_current_store(struct device *dev,
 	list_for_each_entry(rng, &rng_list, list) {
 		if (sysfs_streq(rng->name, buf)) {
 			err = 0;
-			if (rng != current_rng)
-				err = set_current_rng(rng);
+			if (prng) {
+				if (rng != current_prng)
+					err = set_current_rng(rng);
+			} else {
+				if (rng != current_rng)
+					err = set_current_rng(rng);
+			}
 			break;
 		}
 	}
@@ -311,14 +369,28 @@  static ssize_t hwrng_attr_current_store(struct device *dev,
 	return err ? : len;
 }
 
-static ssize_t hwrng_attr_current_show(struct device *dev,
-				       struct device_attribute *attr,
-				       char *buf)
+static ssize_t hwrng_attr_current_store(struct device *dev,
+					struct device_attribute *attr,
+					const char *buf, size_t len)
+{
+	return genrng_attr_current_store(dev, attr, buf, len, false);
+}
+
+static ssize_t hwprng_attr_current_store(struct device *dev,
+					 struct device_attribute *attr,
+					 const char *buf, size_t len)
+{
+	return genrng_attr_current_store(dev, attr, buf, len, true);
+}
+
+static ssize_t genrng_attr_current_show(struct device *dev,
+					struct device_attribute *attr,
+					char *buf, bool prng)
 {
 	ssize_t ret;
 	struct hwrng *rng;
 
-	rng = get_current_rng();
+	rng = get_current_rng(prng);
 	if (IS_ERR(rng))
 		return PTR_ERR(rng);
 
@@ -328,9 +400,24 @@  static ssize_t hwrng_attr_current_show(struct device *dev,
 	return ret;
 }
 
-static ssize_t hwrng_attr_available_show(struct device *dev,
+static ssize_t hwrng_attr_current_show(struct device *dev,
+				       struct device_attribute *attr,
+				       char *buf)
+{
+	return genrng_attr_current_show(dev, attr, buf, false);
+}
+
+static ssize_t hwprng_attr_current_show(struct device *dev,
+					struct device_attribute *attr,
+					char *buf)
+{
+	return genrng_attr_current_show(dev, attr, buf, true);
+}
+
+
+static ssize_t hwgenrng_attr_available_show(struct device *dev,
 					 struct device_attribute *attr,
-					 char *buf)
+					 char *buf, bool prng)
 {
 	int err;
 	struct hwrng *rng;
@@ -340,8 +427,10 @@  static ssize_t hwrng_attr_available_show(struct device *dev,
 		return -ERESTARTSYS;
 	buf[0] = '\0';
 	list_for_each_entry(rng, &rng_list, list) {
-		strlcat(buf, rng->name, PAGE_SIZE);
-		strlcat(buf, " ", PAGE_SIZE);
+		if (rng->isprng == prng) {
+			strlcat(buf, rng->name, PAGE_SIZE);
+			strlcat(buf, " ", PAGE_SIZE);
+		}
 	}
 	strlcat(buf, "\n", PAGE_SIZE);
 	mutex_unlock(&rng_mutex);
@@ -349,6 +438,20 @@  static ssize_t hwrng_attr_available_show(struct device *dev,
 	return strlen(buf);
 }
 
+static ssize_t hwrng_attr_available_show(struct device *dev,
+					 struct device_attribute *attr,
+					 char *buf)
+{
+	return hwgenrng_attr_available_show(dev, attr, buf, false);
+}
+
+static ssize_t hwprng_attr_available_show(struct device *dev,
+					  struct device_attribute *attr,
+					  char *buf)
+{
+	return hwgenrng_attr_available_show(dev, attr, buf, true);
+}
+
 static DEVICE_ATTR(rng_current, S_IRUGO | S_IWUSR,
 		   hwrng_attr_current_show,
 		   hwrng_attr_current_store);
@@ -356,6 +459,13 @@  static DEVICE_ATTR(rng_available, S_IRUGO,
 		   hwrng_attr_available_show,
 		   NULL);
 
+static DEVICE_ATTR(prng_current, S_IRUGO | S_IWUSR,
+		   hwprng_attr_current_show,
+		   hwprng_attr_current_store);
+static DEVICE_ATTR(prng_available, S_IRUGO,
+		   hwprng_attr_available_show,
+		   NULL);
+
 static struct attribute *rng_dev_attrs[] = {
 	&dev_attr_rng_current.attr,
 	&dev_attr_rng_available.attr,
@@ -364,14 +474,35 @@  static struct attribute *rng_dev_attrs[] = {
 
 ATTRIBUTE_GROUPS(rng_dev);
 
+static struct attribute *prng_dev_attrs[] = {
+	&dev_attr_prng_current.attr,
+	&dev_attr_prng_available.attr,
+	NULL
+};
+
+ATTRIBUTE_GROUPS(prng_dev);
+
 static void __exit unregister_miscdev(void)
 {
 	misc_deregister(&rng_miscdev);
+	misc_deregister(&prng_miscdev);
 }
 
 static int __init register_miscdev(void)
 {
-	return misc_register(&rng_miscdev);
+	int err;
+
+	err = misc_register(&rng_miscdev);
+	if (err)
+		return err;
+	err = misc_register(&prng_miscdev);
+	if (err)
+		goto reg_error;
+	else
+		return 0;
+reg_error:
+	misc_deregister(&rng_miscdev);
+	return err;
 }
 
 static int hwrng_fillfn(void *unused)
@@ -381,7 +512,7 @@  static int hwrng_fillfn(void *unused)
 	while (!kthread_should_stop()) {
 		struct hwrng *rng;
 
-		rng = get_current_rng();
+		rng = get_current_rng(false);
 		if (IS_ERR(rng) || !rng)
 			break;
 		mutex_lock(&reading_mutex);
@@ -462,8 +593,8 @@  void hwrng_unregister(struct hwrng *rng)
 	mutex_lock(&rng_mutex);
 
 	list_del(&rng->list);
-	if (current_rng == rng) {
-		drop_current_rng();
+	if (is_current_xrng(rng)) {
+		drop_current_rng(rng->isprng);
 		if (!list_empty(&rng_list)) {
 			struct hwrng *tail;
 
@@ -553,7 +684,8 @@  static int __init hwrng_modinit(void)
 static void __exit hwrng_modexit(void)
 {
 	mutex_lock(&rng_mutex);
-	BUG_ON(current_rng);
+	WARN_ON(current_rng);
+	WARN_ON(current_prng);
 	kfree(rng_buffer);
 	kfree(rng_fillbuf);
 	mutex_unlock(&rng_mutex);
diff --git a/include/linux/hw_random.h b/include/linux/hw_random.h
index 34a0dc1..5a5b8dc 100644
--- a/include/linux/hw_random.h
+++ b/include/linux/hw_random.h
@@ -50,6 +50,7 @@  struct hwrng {
 	struct list_head list;
 	struct kref ref;
 	struct completion cleanup_done;
+	bool isprng;
 };

What do you think about those two solutions ?