Re: [PATCH] md: check command validity early in md_ioctl().

[NeilBrown <neilb@suse.de>]

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

On Wed, 15 Jan 2014 16:58:52 +0100 Nicolas Schichan <nschichan@freebox.fr>
wrote:

> Verify that the cmd parameter passed to md_ioctl() is valid before
> doing anything.
> 
> This fixes mddev->hold_active being set to 0 when an invalid ioctl
> command is passed to md_ioctl() before the array has been configured.
> 
> Clearing mddev->hold_active in that case can lead to a livelock
> situation when an invalid ioctl number is given to md_ioctl() by a
> process when the mddev is currently being opened by another process:
> 
> Process 1				Process 2
> ---------				---------
> 
> md_alloc()
>   mddev_find()
>   -> returns a new mddev with
>      hold_active == UNTIL_IOCTL
>   add_disk()
>   -> sends KOBJ_ADD uevent
> 
> 					(sees KOBJ_ADD uevent for device)
>                     			md_open()
>                     			md_ioctl(INVALID_IOCTL)
>                     			-> returns ENODEV and clears
>                        			   mddev->hold_active
>                     			md_release()
>                       			md_put()
>                       			-> deletes the mddev as
>                          		   hold_active is 0
> 
> md_open()
>   mddev_find()
>   -> returns a newly
>     allocated mddev with
>     mddev->gendisk == NULL
> -> returns with ERESTARTSYS
>    (kernel restarts the open syscall)
> 
> Signed-off-by: Nicolas Schichan <nschichan@freebox.fr>
> ---
> 
> A couple of notes:
> 
> This patch is based on linux 3.13-rc8.
> 
> The following  MD ioctl constants are  defined in md_u.h  but not used
> anywhere else, so are not accepted as valid ioctl commands:
> 
> CLEAR_ARRAY
> SET_DISK_INFO
> WRITE_RAID_INFO
> UNPROTECT_ARRAY
> PROTECT_ARRAY
> 
> 
>  drivers/md/md.c | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 21f4d7f..941ac65 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -6328,6 +6328,32 @@ static int md_getgeo(struct block_device *bdev, struct hd_geometry *geo)
>  	return 0;
>  }
>  
> +static inline bool md_ioctl_valid(unsigned int cmd)
> +{
> +	switch (cmd) {
> +	case ADD_NEW_DISK:
> +	case BLKROSET:
> +	case GET_ARRAY_INFO:
> +	case GET_BITMAP_FILE:
> +	case GET_DISK_INFO:
> +	case HOT_ADD_DISK:
> +	case HOT_REMOVE_DISK:
> +	case PRINT_RAID_DEBUG:
> +	case RAID_AUTORUN:
> +	case RAID_VERSION:
> +	case RESTART_ARRAY_RW:
> +	case RUN_ARRAY:
> +	case SET_ARRAY_INFO:
> +	case SET_BITMAP_FILE:
> +	case SET_DISK_FAULTY:
> +	case STOP_ARRAY:
> +	case STOP_ARRAY_RO:
> +		return true;
> +	default:
> +		return false;
> +	}
> +}
> +
>  static int md_ioctl(struct block_device *bdev, fmode_t mode,
>  			unsigned int cmd, unsigned long arg)
>  {
> @@ -6336,6 +6362,9 @@ static int md_ioctl(struct block_device *bdev, fmode_t mode,
>  	struct mddev *mddev = NULL;
>  	int ro;
>  
> +	if (!md_ioctl_valid(cmd))
> +		return -ENOTTY;
> +
>  	switch (cmd) {
>  	case RAID_VERSION:
>  	case GET_ARRAY_INFO:


Patch applied - thanks!

NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]