Re: [PATCH] floppy: Do not copy a kernel pointer to user memory in FDGETPRM ioctl

[Andy Whitcroft]

On Wed, Mar 07, 2018 at 04:02:45PM -0800, Brian Belleville wrote:
> The final field of a floppy_struct is the field "name", which is a
> pointer to a string in kernel memory. The kernel pointer should not be
> copied to user memory. The FDGETPRM ioctl copies a floppy_struct to
> user memory, including the "name" field. This pointer cannot be used
> by the user, and it will leak a kernel address to user-space, which
> will reveal the location of kernel code and data and undermine KASLR
> protection. Instead, copy the floppy_struct except for the "name"
> field.
> 
> Signed-off-by: Brian Belleville <bbellevi@xxxxxxx>
> ---
>  drivers/block/floppy.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c
> index eae484a..4d4a422 100644
> --- a/drivers/block/floppy.c
> +++ b/drivers/block/floppy.c
> @@ -3470,6 +3470,7 @@ static int fd_locked_ioctl(struct block_device *bdev, fmode_t mode, unsigned int
>  					  (struct floppy_struct **)&outparam);
>  		if (ret)
>  			return ret;
> +		size = offsetof(struct floppy_struct, name);
>  		break;
>  	case FDMSGON:
>  		UDP->flags |= FTD_MSG;

I am not sure it is reasonable to simply set size here to the length of the
valid data.  Though in the real world everyonne should be using the defines
and those should include the full length, the code itself does not require
this, it only prevents overly long reads.  So I think it is possible to do
this read with a shorter userspace buffer; with this change we would
then write beyond the end of the buffer.

This also seems to introduce a slight behavioural difference between the
primary and compat calls.  The compat call already elides the name but it
also is copying into a new structure for return and this is pre-cleared,
so the name will always be null for the compat case and undefined for
the primary ioctl.

Perhaps the below patch would be more appropriate.

-apw