Re: [PATCH] floppy: hide invalid floppy disk types

[Denis Efremov]

On 12/9/19 12:04 AM, Philip K. wrote:
> Denis Efremov <yefremov.denis@xxxxxxxxx> writes:
> 
>> Hi,
>>
>> On 08.12.2019 22:45, Moritz MÃller wrote:
>>> In some cases floppy disks are being indexed, even though no actual
>>> device exists. In our case this was caused by the CMOS-RAM having a few
>>> peculiar bits. This caused a non-existent floppy disk of the type 13 to
>>> be registered as an possibly mountable device, even though it could not
>>> be mounted by any user.
>>>
>>> We believe this to be an instance of this bug, as we had similar logs
>>> and issues:
>>>
>>>  https://bugzilla.kernel.org/show_bug.cgi?id=13486
>>>  https://bugs.launchpad.net/ubuntu/+source/linux/+bug/384579

Well, this is a ten years old bug. It seems like that time it was decided
to fix the problem either by blacklisting the driver or by disabling the
BIOS option. I doubt that many people are facing this problem since then.
I think that more-or-less modern motherboards don't have this issue.

>>>
>>> This patch adds the option FLOPPY_ALLOW_UNKNOWN_TYPES to prevent the
>>> additional check that fixed the issue on our reference system, and
>>> increases the startup time of affected systems by over a minute.
>>
>> Does driver blacklisting solves your problem? Or you have real floppy drives in
>> your system along with these "spurious" ones?
> 
> No there were not, and blacklisting would solve the bug too. We just
> thought that fixing the bug this way would prevent the issue from
> appearing in the first place on systems that have the floppy module
> enabled, in the first place.

Hmm, I would say that driver blacklisting is a more proper solution in
this case. I doubt there are people with this issue and real floppy drives
in their setup. Altering the default driver's initialization scheme seems
superfluous to me. This will force users (if there are ones) who depends on this
behavior to rebuild the kernel. blacklisting doesn't require kernel rebuild.

> 
>>> Co-developed-by: Philip K. <philip@xxxxxxxxxxxx>
>>> Signed-off-by: Philip K. <philip@xxxxxxxxxxxx>
>>> Signed-off-by: Moritz MÃller <moritzm.mueller@xxxxxxxxx>
>>> ---
>>>  drivers/block/Kconfig  | 10 ++++++++++
>>>  drivers/block/floppy.c |  6 ++++++
>>>  2 files changed, 16 insertions(+)
>>>
>>> diff --git a/drivers/block/Kconfig b/drivers/block/Kconfig
>>> index 1bb8ec575352..9e6b32c50b67 100644
>>> --- a/drivers/block/Kconfig
>>> +++ b/drivers/block/Kconfig
>>> @@ -72,6 +72,16 @@ config AMIGA_Z2RAM
>>>  	  To compile this driver as a module, choose M here: the
>>>  	  module will be called z2ram.
>>>  
>>> +config FLOPPY_ALLOW_UNKNOWN_TYPES
>>> +	bool "Allow floppy disks of unknown type to be registered."
>>> +	default n
>>> +	help
>>> +	  Select this option if you want the Kernel to register floppy
>>> +	  disks of an unknown type.
>>> +
>>> +	  This should usually not be enabled, because of cases where the
>>> +	  system falsely recognizes a non-existent floppy disk as mountable.
>>> +
>>>  config CDROM
>>>  	tristate
>>>  	select BLK_SCSI_REQUEST
>>> diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c
>>> index 485865fd0412..9439444d46d0 100644
>>> --- a/drivers/block/floppy.c
>>> +++ b/drivers/block/floppy.c
>>> @@ -3949,7 +3949,9 @@ static void __init config_types(void)
>>>  			} else
>>>  				allowed_drive_mask &= ~(1 << drive);
>>>  		} else {
>>> +#ifdef CONFIG_FLOPPY_ALLOW_UNKNOWN_TYPES
>>>  			params = &default_drive_params[0].params;
>>> +#endif
>>
>> You can't just skip it with ifdef. This will result in uninitialized
>> pointer dereference down the code.
>>
>> 		struct floppy_drive_params *params;
>> 		...
>>
>> 		if (type < ARRAY_SIZE(default_drive_params)) {
>> 			...
>> 		} else {
>> #ifdef CONFIG_FLOPPY_ALLOW_UNKNOWN_TYPES
>> 			params = &default_drive_params[0].params;
>> #endif
>> 			...
>> 		}
>> 		...
>> 		*UDP = *params; // << HERE
> 
> Oops, you're right, will fix.
> 
>>>  			snprintf(temparea, sizeof(temparea),
>>>  				 "unknown type %d (usb?)", type);
>>>  			name = temparea;
>>> @@ -4518,6 +4520,10 @@ static bool floppy_available(int drive)
>>>  		return false;
>>>  	if (fdc_state[FDC(drive)].version == FDC_NONE)
>>>  		return false;
>>> +#ifndef CONFIG_FLOPPY_ALLOW_UNKNOWN_TYPES
>>> +	if (type >= ARRAY_SIZE(default_drive_params))
>>> +		return false;
>>> +#endif
>>>  	return true;
>>>  }
>>>  
>>>
>>
>> Thanks,
>> Denis
>