linux-kernel - Re: [PATCH] block:Add proper error checking to genhd.c for the function,add

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [day] [month] [year] [list]

Date:	Tue, 10 Feb 2015 14:04:51 -0700
From:	Jens Axboe <axboe@...nel.dk>
To:	Nicholas Krause <xerofoify@...il.com>
CC:	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] block:Add proper error checking to genhd.c for the function,add_disk

On 02/09/2015 06:22 PM, Nicholas Krause wrote:
> Adds error handling to the function add_disk. Error checking is
> added by removing the calls to WARN_ON when there is a failure in
> various places in the function to proper return statements with
> a error code. Further more due to this we must also add a statement
> to return 0 at the end of the function to signal success to the
> caller of the function to notify it that the function,add_disk has
> succeeded and change the function's return type to a int now to
> allow returning of error codes  for the function,add_disk to be
> allowed.

There's a lot wrong with this still:

> @@ -595,10 +594,9 @@ void add_disk(struct gendisk *disk)
>   	disk->flags |= GENHD_FL_UP;
>
>   	retval = blk_alloc_devt(&disk->part0, &devt);
> -	if (retval) {
> -		WARN_ON(1);
> -		return;
> -	}
> +	if (retval)
> +		return -ENOMEM;

Why isn't that returning retval?

> +
>   	disk_to_dev(disk)->devt = devt;
>
>   	/* ->major and ->first_minor aren't supposed to be
> @@ -622,13 +620,16 @@ void add_disk(struct gendisk *disk)
>   	 * Take an extra ref on queue which will be put on disk_release()
>   	 * so that it sticks around as long as @disk is there.
>   	 */
> -	WARN_ON_ONCE(!blk_get_queue(disk->queue));
> +	if (!blk_get_queue(disk->queue))
> +		return -ENODEV;

You just leaked the devt alloc.

>   	retval = sysfs_create_link(&disk_to_dev(disk)->kobj, &bdi->dev->kobj,
>   				   "bdi");
> -	WARN_ON(retval);
> -
> +	if (retval)
> +		return -EFAULT;

Leaked devt alloc and queue ref.

On top of that, some of the functions that add_disk() calls can fail, 
yet it doesn't check for those. And the most major part of this is 
ensuring that all callders of add_disk() properly check and handle the 
error it will pass back, that is completely ignored. Just making 
add_disk() error and unroll itself is the smaller part of the task.


-- 
Jens Axboe

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/