[PATCH 1/2] chardev: fix an overlap misjudgement case in __register_chrdev_region()

* [PATCH 1/2] chardev: fix an overlap misjudgement case in __register_chrdev_region()
@ 2019-02-12  8:47 Chengguang Xu
  2019-02-12  8:47 ` [PATCH 2/2] chardev: showing minor range for chardev in the output of /proc/devices Chengguang Xu
  2019-02-12  9:01 ` [PATCH 1/2] chardev: fix an overlap misjudgement case in __register_chrdev_region() Greg KH
  0 siblings, 2 replies; 8+ messages in thread
From: Chengguang Xu @ 2019-02-12  8:47 UTC (permalink / raw)
  To: gregkh; +Cc: viro, linux-kernel, stable, Chengguang Xu

Current overlap check of minor range cannot correctly
handle a case which is baseminor < existing baseminor &&
baseminor + minorct > existing baseminor + minorct.
Fix it and meanwhile do some code cleanups.

Fixes: 01d553d0fe9f90 ("Chardev checking of overlapping ranges")
Signed-off-by: Chengguang Xu <cgxu519@gmx.com>
Cc: stable@vger.kernel.org
---
 fs/char_dev.c | 94 ++++++++++++++++++++++++---------------------------
 1 file changed, 44 insertions(+), 50 deletions(-)

diff --git a/fs/char_dev.c b/fs/char_dev.c
index a279c58fe360..b25b1da097d5 100644
--- a/fs/char_dev.c
+++ b/fs/char_dev.c
@@ -88,86 +88,80 @@ static int find_dynamic_major(void)
 /*
  * Register a single major with a specified minor range.
  *
- * If major == 0 this functions will dynamically allocate a major and return
- * its number.
- *
- * If major > 0 this function will attempt to reserve the passed range of
- * minors and will return zero on success.
+ * If major == 0 this function will dynamically allocate an unused major,
+ * otherwise attempt to reserve the range of minors with given major.
  *
- * Returns a -ve errno on failure.
  */
 static struct char_device_struct *
 __register_chrdev_region(unsigned int major, unsigned int baseminor,
 			   int minorct, const char *name)
 {
-	struct char_device_struct *cd, **cp;
-	int ret = 0;
+	struct char_device_struct *new, *curr, *prev = NULL;
+	int ret = -EBUSY;
 	int i;
 
-	cd = kzalloc(sizeof(struct char_device_struct), GFP_KERNEL);
-	if (cd == NULL)
+	if (major >= CHRDEV_MAJOR_MAX) {
+		pr_err("CHRDEV \"%s\" major requested (%u) is greater than the maximum (%u)\n",
+		       name, major, CHRDEV_MAJOR_MAX-1);
+		return  ERR_PTR(-EINVAL);
+	}
+
+	if (minorct > MINORMASK + 1 - baseminor) {
+		pr_err("CHRDEV \"%s\" minor range requested (%u-%u) is out of range of maximum range (%u-%u) for a single major\n",
+		       name, baseminor, baseminor + minorct - 1, 0, MINORMASK);
+		return ERR_PTR(-EINVAL);
+	}
+
+	new = kzalloc(sizeof(struct char_device_struct), GFP_KERNEL);
+	if (new == NULL)
 		return ERR_PTR(-ENOMEM);
 
 	mutex_lock(&chrdevs_lock);
 
 	if (major == 0) {
-		ret = find_dynamic_major();
-		if (ret < 0) {
+		major = find_dynamic_major();
+		if (major < 0) {
 			pr_err("CHRDEV \"%s\" dynamic allocation region is full\n",
 			       name);
 			goto out;
 		}
-		major = ret;
 	}
 
-	if (major >= CHRDEV_MAJOR_MAX) {
-		pr_err("CHRDEV \"%s\" major requested (%u) is greater than the maximum (%u)\n",
-		       name, major, CHRDEV_MAJOR_MAX-1);
-		ret = -EINVAL;
-		goto out;
-	}
-
-	cd->major = major;
-	cd->baseminor = baseminor;
-	cd->minorct = minorct;
-	strlcpy(cd->name, name, sizeof(cd->name));
-
 	i = major_to_index(major);
+	for (curr = chrdevs[i]; curr; prev = curr, curr = curr->next) {
+		if (curr->major < major)
+			continue;
 
-	for (cp = &chrdevs[i]; *cp; cp = &(*cp)->next)
-		if ((*cp)->major > major ||
-		    ((*cp)->major == major &&
-		     (((*cp)->baseminor >= baseminor) ||
-		      ((*cp)->baseminor + (*cp)->minorct > baseminor))))
+		if (curr->major > major)
 			break;
 
-	/* Check for overlapping minor ranges.  */
-	if (*cp && (*cp)->major == major) {
-		int old_min = (*cp)->baseminor;
-		int old_max = (*cp)->baseminor + (*cp)->minorct - 1;
-		int new_min = baseminor;
-		int new_max = baseminor + minorct - 1;
+		if (curr->baseminor + curr->minorct <= baseminor)
+			continue;
 
-		/* New driver overlaps from the left.  */
-		if (new_max >= old_min && new_max <= old_max) {
-			ret = -EBUSY;
-			goto out;
-		}
+		if (curr->baseminor >= baseminor + minorct)
+			break;
 
-		/* New driver overlaps from the right.  */
-		if (new_min <= old_max && new_min >= old_min) {
-			ret = -EBUSY;
-			goto out;
-		}
+		goto out;
+	}
+
+	new->major = major;
+	new->baseminor = baseminor;
+	new->minorct = minorct;
+	strlcpy(new->name, name, sizeof(new->name));
+
+	if (!prev) {
+		new->next = curr;
+		chrdevs[i] = new;
+	} else {
+		new->next = prev->next;
+		prev->next = new;
 	}
 
-	cd->next = *cp;
-	*cp = cd;
 	mutex_unlock(&chrdevs_lock);
-	return cd;
+	return new;
 out:
 	mutex_unlock(&chrdevs_lock);
-	kfree(cd);
+	kfree(new);
 	return ERR_PTR(ret);
 }
 
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread