Bug in chardev checking of overlapping ranges(send again as TEXT/PLAIN)

Bug in chardev checking of overlapping ranges(send again as TEXT/PLAIN)

[Gavin Chang]

[-- Attachment #1: Type: text/plain, Size: 1313 bytes --]

Hi,

I'm a new person to learn Linux kernel.

In fs/char_dev.c function __register_chrdev_region(), there is the
following code:

for (cp = &chrdevs[i]; *cp; cp = &(*cp)->next)
if ((*cp)->major > major ||
    ((*cp)->major == major &&
     (((*cp)->baseminor >= baseminor) ||
      ((*cp)->baseminor + (*cp)->minorct > baseminor))))
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;

/* New driver overlaps from the left.  */
if (new_max >= old_min && new_max <= old_max) {
ret = -EBUSY;
goto out;
}

/* New driver overlaps from the right.  */
if (new_min <= old_max && new_min >= old_min) {
ret = -EBUSY;
goto out;
}
}

I think there is a bug in checking of overlapping ranges.
For example, driver X has registered with major=x and minor=1-3, and
__register_chrdev_region() will allow driver Y to register with
major=x and minor=0-4.
The minor of driver Y will not meet the two if statements:
if (new_max >= old_min && new_max <= old_max)  and if (new_min <=
old_max && new_min >= old_min)

The attached is my patch to repair this problem.
And there is a related commit 01d553d0fe9f90a132c5ff494872be8d4126be1e
for reference.

[-- Attachment #2: char_dev.patch --]
[-- Type: text/x-patch, Size: 1200 bytes --]

diff --git a/fs/char_dev.c b/fs/char_dev.c
index 24b1425..d65765e 100644
--- a/fs/char_dev.c
+++ b/fs/char_dev.c
@@ -107,29 +107,14 @@ __register_chrdev_region(unsigned int major, unsigned int baseminor,
 
 	for (cp = &chrdevs[i]; *cp; cp = &(*cp)->next)
 		if ((*cp)->major > major ||
-		    ((*cp)->major == major &&
-		     (((*cp)->baseminor >= baseminor) ||
-		      ((*cp)->baseminor + (*cp)->minorct > baseminor))))
+		    ((*cp)->major == major && ((*cp)->baseminor + (*cp)->minorct > baseminor)))
 			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;
-
-		/* New driver overlaps from the left.  */
-		if (new_max >= old_min && new_max <= old_max) {
-			ret = -EBUSY;
-			goto out;
-		}
-
-		/* New driver overlaps from the right.  */
-		if (new_min <= old_max && new_min >= old_min) {
-			ret = -EBUSY;
-			goto out;
-		}
+	if (*cp && (*cp)->major == major &&
+	    (*cp)->baseminor < baseminor + minorct) {
+               ret = -EBUSY;
+               goto out;
 	}
 
 	cd->next = *cp;

Re: Bug in chardev checking of overlapping ranges(send again as TEXT/PLAIN)

[Omar Sandoval]

On Wed, May 25, 2016 at 10:53:36PM +0800, Gavin Chang wrote:
> Hi,
> 
> I'm a new person to learn Linux kernel.
> 
> In fs/char_dev.c function __register_chrdev_region(), there is the
> following code:
> 
> for (cp = &chrdevs[i]; *cp; cp = &(*cp)->next)
> if ((*cp)->major > major ||
>     ((*cp)->major == major &&
>      (((*cp)->baseminor >= baseminor) ||
>       ((*cp)->baseminor + (*cp)->minorct > baseminor))))
> 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;
> 
> /* New driver overlaps from the left.  */
> if (new_max >= old_min && new_max <= old_max) {
> ret = -EBUSY;
> goto out;
> }
> 
> /* New driver overlaps from the right.  */
> if (new_min <= old_max && new_min >= old_min) {
> ret = -EBUSY;
> goto out;
> }
> }
> 
> I think there is a bug in checking of overlapping ranges.
> For example, driver X has registered with major=x and minor=1-3, and
> __register_chrdev_region() will allow driver Y to register with
> major=x and minor=0-4.
> The minor of driver Y will not meet the two if statements:
> if (new_max >= old_min && new_max <= old_max)  and if (new_min <=
> old_max && new_min >= old_min)
> 
> The attached is my patch to repair this problem.
> And there is a related commit 01d553d0fe9f90a132c5ff494872be8d4126be1e
> for reference.

> diff --git a/fs/char_dev.c b/fs/char_dev.c
> index 24b1425..d65765e 100644
> --- a/fs/char_dev.c
> +++ b/fs/char_dev.c
> @@ -107,29 +107,14 @@ __register_chrdev_region(unsigned int major, unsigned int baseminor,
>  
>  	for (cp = &chrdevs[i]; *cp; cp = &(*cp)->next)
>  		if ((*cp)->major > major ||
> -		    ((*cp)->major == major &&
> -		     (((*cp)->baseminor >= baseminor) ||
> -		      ((*cp)->baseminor + (*cp)->minorct > baseminor))))
> +		    ((*cp)->major == major && ((*cp)->baseminor + (*cp)->minorct > baseminor)))
>  			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;
> -
> -		/* New driver overlaps from the left.  */
> -		if (new_max >= old_min && new_max <= old_max) {
> -			ret = -EBUSY;
> -			goto out;
> -		}
> -
> -		/* New driver overlaps from the right.  */
> -		if (new_min <= old_max && new_min >= old_min) {
> -			ret = -EBUSY;
> -			goto out;
> -		}
> +	if (*cp && (*cp)->major == major &&
> +	    (*cp)->baseminor < baseminor + minorct) {
> +               ret = -EBUSY;
> +               goto out;
>  	}
>  
>  	cd->next = *cp;

Gavin,

You're going to want to read Documentation/SubmittingPatches. Basically,
you want to use git commit -s, git format-patch, and git send-email
rather than attachments.

Thanks,
-- 
Omar