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

[Corentin Labbe <clabbe.montjoie@gmail.com>]

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:
--- 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 ?

Regards
Corentin Labbe