[PATCH] floppy: hide invalid floppy disk types

[PATCH] floppy: hide invalid floppy disk types

[Moritz Müller]

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

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.

Co-developed-by: Philip K. <philip@warpmail.net>
Signed-off-by: Philip K. <philip@warpmail.net>
Signed-off-by: Moritz Müller <moritzm.mueller@posteo.de>
---
 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
 			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;
 }
 
-- 
2.20.1

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

[Denis Efremov]

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
> 
> 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?

> 
> Co-developed-by: Philip K. <philip@warpmail.net>
> Signed-off-by: Philip K. <philip@warpmail.net>
> Signed-off-by: Moritz Müller <moritzm.mueller@posteo.de>
> ---
>  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

>  			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

[PATCH v3] floppy: hide invalid floppy disk types

[Moritz Müller]

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 (in
our case) 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:

 https://bugzilla.kernel.org/show_bug.cgi?id=13486
 https://bugs.launchpad.net/ubuntu/+source/linux/+bug/384579

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.

Co-developed-by: Philip K. <philip@warpmail.net>
Signed-off-by: Philip K. <philip@warpmail.net>
Signed-off-by: Moritz Müller <moritzm.mueller@posteo.de>
---
 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,11 @@ 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;
+#else
+ 			params = UDP;
+#endif
 			snprintf(temparea, sizeof(temparea),
 				 "unknown type %d (usb?)", type);
 			name = temparea;
@@ -4518,7 +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 (UDP->cmos >= ARRAY_SIZE(default_drive_params))
+		return false;
+#endif
 	return true;
 }
 
-- 
2.20.1

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

[Denis Efremov]

Hi,

On 12/9/19 12:32 PM, 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 (in
> our case) 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:
> 
>  https://bugzilla.kernel.org/show_bug.cgi?id=13486
>  https://bugs.launchpad.net/ubuntu/+source/linux/+bug/384579
> 
> 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.
> 
> Co-developed-by: Philip K. <philip@warpmail.net>
> Signed-off-by: Philip K. <philip@warpmail.net>
> Signed-off-by: Moritz Müller <moritzm.mueller@posteo.de>

Thank you for the patch!

Have you tested your patch with FLOPPY_ALLOW_UNKNOWN_TYPES and without
FLOPPY_ALLOW_UNKNOWN_TYPES?

I will answer about motivation for this change in V2 branch of the patch.

> ---
>  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,11 @@ 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;
> +#else
> + 			params = UDP;
> +#endif
>  			snprintf(temparea, sizeof(temparea),
>  				 "unknown type %d (usb?)", type);
>  			name = temparea;

Maybe just skip the else branch completely here? This will omit
snprintf, following if (name) block and UDP update.

+#ifdef CONFIG_FLOPPY_ALLOW_UNKNOWN_TYPES
                        params = &default_drive_params[0].params;
                        snprintf(temparea, sizeof(temparea),
                                 "unknown type %d (usb?)", type);
                        name = temparea;
+#else
+                       continue;
+#endif


> @@ -4518,7 +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 (UDP->cmos >= ARRAY_SIZE(default_drive_params))
> +		return false;
> +#endif
>  	return true;
>  }
>  
> 

Thanks,
Denis

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

[Denis Efremov]

On 12/9/19 12:04 AM, Philip K. wrote:
> Denis Efremov <yefremov.denis@gmail.com> 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@warpmail.net>
>>> Signed-off-by: Philip K. <philip@warpmail.net>
>>> Signed-off-by: Moritz Müller <moritzm.mueller@posteo.de>
>>> ---
>>>  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
> 

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

[Philip K.]

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


> 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.

As long as major distributions like Ubuntu ship the floppy module, there
are enough people who could be affected by this peculiar behaviour.
While I agree that blacklisting the module would be more elegant, I
still think that a patch that goes in this direction could help more
people, especially those who don't want or cannot solve kernel-related
issues.

> This will force users (if there are ones) who depends on this behavior
> to rebuild the kernel. blacklisting doesn't require kernel rebuild.

Are there floppy disks of unknown types? Our patch is intentionally
conservative: We won't hide false negatives. If the motherboard reports
an non-existent disk

If you're ready to think about it, we could consider extending the patch
to un-register the device if it can recognise that it's (probably) not
real. In our case, for example, fdisk reported that fd0 had a size of
4k, something think is a strong indicator that something's not right.

Alternatively, we could look into what the comment

	/* FIXME: additional physical CMOS drive detection should go here */

would imply. This particular bug can only affect fd0 and fd1, so if we
spent some more time, we could find something.

-- 
	With kind regards,
	Philip K.

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

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

[kbuild test robot]

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

Hi "Moritz,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on block/for-next]
[also build test ERROR on linux/master linus/master v5.4 next-20191208]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Moritz-M-ller/floppy-hide-invalid-floppy-disk-types/20191209-035056
base:   https://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git for-next
config: x86_64-rhel (attached as .config)
compiler: gcc-7 (Debian 7.5.0-1) 7.5.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers//block/floppy.c: In function 'floppy_available':
>> drivers//block/floppy.c:4524:6: error: 'type' undeclared (first use in this function); did you mean 'true'?
     if (type >= ARRAY_SIZE(default_drive_params))
         ^~~~
         true
   drivers//block/floppy.c:4524:6: note: each undeclared identifier is reported only once for each function it appears in

vim +4524 drivers//block/floppy.c

  4516	
  4517	static bool floppy_available(int drive)
  4518	{
  4519		if (!(allowed_drive_mask & (1 << drive)))
  4520			return false;
  4521		if (fdc_state[FDC(drive)].version == FDC_NONE)
  4522			return false;
  4523	#ifndef CONFIG_FLOPPY_ALLOW_UNKNOWN_TYPES
> 4524		if (type >= ARRAY_SIZE(default_drive_params))
  4525			return false;
  4526	#endif
  4527		return true;
  4528	}
  4529	

---
0-DAY kernel test infrastructure                 Open Source Technology Center
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 44037 bytes --]