[PATCH v3 0/4] floppy: suppress UBSAN warning in setup_rw_floppy()

[PATCH v3 0/4] floppy: suppress UBSAN warning in setup_rw_floppy()

[Denis Efremov]

These patches are based on Willy's cleanup patches
https://lkml.org/lkml/2020/3/31/609.

The first patch removes pr_cont() in setup_DMA() and prints the contents of
cmd buffer with print_hex_dump(). The last patch also touches these lines
and changes cmd buffer to fullcmd. The 2,3 patches introduce defines to
make it more clear why cmd_count in struct floppy_raw_cmd allows
out-of-bounds access for cmd, reply_count, reply fields. Last patch
handles the warning.

There is the checkpatch warning because of the line length 81 > 80 in
the definition of valid_floppy_drive_params(). This function
intentionally uses autodetect[8] as argument to highlight that this
is exactly the autodetect field from structs floppy_drive_params and
compat_floppy_drive_params. I think that this warning is not a big
problem since next function definition is 85 chars long and whole
driver requires "restyling".

Changes in v3:
 - fix indentation in the second patch
 - remove FD_RAW_REPLY_SIZE from print_hex_dump() in the third patch

Changes in v2:
 - add FD_RAW_CMD_FULLSIZE to the second patch
 - add union with fullcmd to struct floppy_raw_cmd in the last patch

Denis Efremov (4):
  floppy: use print_hex_dump() in setup_DMA()
  floppy: add FD_AUTODETECT_SIZE define for struct floppy_drive_params
  floppy: add defines for sizes of cmd & reply buffers of floppy_raw_cmd
  floppy: suppress UBSAN warning in setup_rw_floppy()

 drivers/block/floppy.c  | 39 ++++++++++++++-------------------------
 include/uapi/linux/fd.h | 26 ++++++++++++++++++++++----
 2 files changed, 36 insertions(+), 29 deletions(-)

-- 
2.25.3

[PATCH v3 1/4] floppy: use print_hex_dump() in setup_DMA()

[Denis Efremov]

Remove pr_cont() and use print_hex_dump() in setup_DMA() to print the
contents of the cmd buffer.

Suggested-by: Joe Perches <joe@perches.com>
Signed-off-by: Denis Efremov <efremov@linux.com>
---
 drivers/block/floppy.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c
index 2817170dd403..3ab6e804b5ec 100644
--- a/drivers/block/floppy.c
+++ b/drivers/block/floppy.c
@@ -1069,12 +1069,9 @@ static void setup_DMA(void)
 	unsigned long f;
 
 	if (raw_cmd->length == 0) {
-		int i;
-
-		pr_info("zero dma transfer size:");
-		for (i = 0; i < raw_cmd->cmd_count; i++)
-			pr_cont("%x,", raw_cmd->cmd[i]);
-		pr_cont("\n");
+		print_hex_dump(KERN_INFO, "zero dma transfer size: ",
+			       DUMP_PREFIX_NONE, 16, 1,
+			       raw_cmd->cmd, raw_cmd->cmd_count, false);
 		cont->done(0);
 		fdc_state[current_fdc].reset = 1;
 		return;
-- 
2.25.3

Re: [PATCH v3 1/4] floppy: use print_hex_dump() in setup_DMA()

[Christoph Hellwig]

On Fri, May 01, 2020 at 04:44:13PM +0300, Denis Efremov wrote:
> Remove pr_cont() and use print_hex_dump() in setup_DMA() to print the
> contents of the cmd buffer.
> 
> Suggested-by: Joe Perches <joe@perches.com>
> Signed-off-by: Denis Efremov <efremov@linux.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH v3 2/4] floppy: add FD_AUTODETECT_SIZE define for struct floppy_drive_params

[Denis Efremov]

Use FD_AUTODETECT_SIZE for autodetect buffer size in struct
floppy_drive_params instead of a magic number.

Signed-off-by: Denis Efremov <efremov@linux.com>
---
 drivers/block/floppy.c  | 9 +++++----
 include/uapi/linux/fd.h | 5 ++++-
 2 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c
index 3ab6e804b5ec..b82b3d38b834 100644
--- a/drivers/block/floppy.c
+++ b/drivers/block/floppy.c
@@ -2073,7 +2073,8 @@ static int next_valid_format(int drive)
 
 	probed_format = drive_state[drive].probed_format;
 	while (1) {
-		if (probed_format >= 8 || !drive_params[drive].autodetect[probed_format]) {
+		if (probed_format >= FD_AUTODETECT_SIZE ||
+		    !drive_params[drive].autodetect[probed_format]) {
 			drive_state[drive].probed_format = 0;
 			return 1;
 		}
@@ -3442,13 +3443,13 @@ static int fd_getgeo(struct block_device *bdev, struct hd_geometry *geo)
 	return 0;
 }
 
-static bool valid_floppy_drive_params(const short autodetect[8],
+static bool valid_floppy_drive_params(const short autodetect[FD_AUTODETECT_SIZE],
 		int native_format)
 {
 	size_t floppy_type_size = ARRAY_SIZE(floppy_type);
 	size_t i = 0;
 
-	for (i = 0; i < 8; ++i) {
+	for (i = 0; i < FD_AUTODETECT_SIZE; ++i) {
 		if (autodetect[i] < 0 ||
 		    autodetect[i] >= floppy_type_size)
 			return false;
@@ -3673,7 +3674,7 @@ struct compat_floppy_drive_params {
 	struct floppy_max_errors max_errors;
 	char		flags;
 	char		read_track;
-	short		autodetect[8];
+	short		autodetect[FD_AUTODETECT_SIZE];
 	compat_int_t	checkfreq;
 	compat_int_t	native_format;
 };
diff --git a/include/uapi/linux/fd.h b/include/uapi/linux/fd.h
index 90fb94712c41..3f6b7be4c096 100644
--- a/include/uapi/linux/fd.h
+++ b/include/uapi/linux/fd.h
@@ -172,7 +172,10 @@ struct floppy_drive_params {
  * used in succession to try to read the disk. If the FDC cannot lock onto
  * the disk, the next format is tried. This uses the variable 'probing'.
  */
-	short autodetect[8];		/* autodetected formats */
+
+#define FD_AUTODETECT_SIZE 8
+
+	short autodetect[FD_AUTODETECT_SIZE]; /* autodetected formats */
 	
 	int checkfreq; /* how often should the drive be checked for disk 
 			* changes */
-- 
2.25.3

Re: [PATCH v3 2/4] floppy: add FD_AUTODETECT_SIZE define for struct floppy_drive_params

[Christoph Hellwig]

On Fri, May 01, 2020 at 04:44:14PM +0300, Denis Efremov wrote:
> Use FD_AUTODETECT_SIZE for autodetect buffer size in struct
> floppy_drive_params instead of a magic number.

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH v3 3/4] floppy: add defines for sizes of cmd & reply buffers of floppy_raw_cmd

[Denis Efremov]

Use FD_RAW_CMD_SIZE, FD_RAW_REPLY_SIZE defines instead of magic numbers
for cmd & reply buffers of struct floppy_raw_cmd. Remove local to
floppy.c MAX_REPLIES define, as it is now FD_RAW_REPLY_SIZE.
FD_RAW_CMD_FULLSIZE added as we allow command to also fill reply_count
and reply fields.

Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Denis Efremov <efremov@linux.com>
---
 drivers/block/floppy.c  | 19 +++++--------------
 include/uapi/linux/fd.h | 14 ++++++++++++--
 2 files changed, 17 insertions(+), 16 deletions(-)

diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c
index b82b3d38b834..9e098d53b046 100644
--- a/drivers/block/floppy.c
+++ b/drivers/block/floppy.c
@@ -337,8 +337,7 @@ static bool initialized;
 /*
  * globals used by 'result()'
  */
-#define MAX_REPLIES 16
-static unsigned char reply_buffer[MAX_REPLIES];
+static unsigned char reply_buffer[FD_RAW_REPLY_SIZE];
 static int inr;		/* size of reply buffer, when called from interrupt */
 #define ST0		0
 #define ST1		1
@@ -1162,7 +1161,7 @@ static int result(int fdc)
 	int i;
 	int status = 0;
 
-	for (i = 0; i < MAX_REPLIES; i++) {
+	for (i = 0; i < FD_RAW_REPLY_SIZE; i++) {
 		status = wait_til_ready(fdc);
 		if (status < 0)
 			break;
@@ -3079,7 +3078,7 @@ static void raw_cmd_done(int flag)
 		raw_cmd->flags |= FD_RAW_HARDFAILURE;
 	} else {
 		raw_cmd->reply_count = inr;
-		if (raw_cmd->reply_count > MAX_REPLIES)
+		if (raw_cmd->reply_count > FD_RAW_REPLY_SIZE)
 			raw_cmd->reply_count = 0;
 		for (i = 0; i < raw_cmd->reply_count; i++)
 			raw_cmd->reply[i] = reply_buffer[i];
@@ -3190,18 +3189,10 @@ static int raw_cmd_copyin(int cmd, void __user *param,
 	if (ret)
 		return -EFAULT;
 	param += sizeof(struct floppy_raw_cmd);
-	if (ptr->cmd_count > 33)
-			/* the command may now also take up the space
-			 * initially intended for the reply & the
-			 * reply count. Needed for long 82078 commands
-			 * such as RESTORE, which takes ... 17 command
-			 * bytes. Murphy's law #137: When you reserve
-			 * 16 bytes for a structure, you'll one day
-			 * discover that you really need 17...
-			 */
+	if (ptr->cmd_count > FD_RAW_CMD_FULLSIZE)
 		return -EINVAL;
 
-	for (i = 0; i < 16; i++)
+	for (i = 0; i < FD_RAW_REPLY_SIZE; i++)
 		ptr->reply[i] = 0;
 	ptr->resultcode = 0;
 
diff --git a/include/uapi/linux/fd.h b/include/uapi/linux/fd.h
index 3f6b7be4c096..2e9c2c1c18e6 100644
--- a/include/uapi/linux/fd.h
+++ b/include/uapi/linux/fd.h
@@ -360,10 +360,20 @@ struct floppy_raw_cmd {
 	int buffer_length; /* length of allocated buffer */
 
 	unsigned char rate;
+
+#define FD_RAW_CMD_SIZE 16
+#define FD_RAW_REPLY_SIZE 16
+#define FD_RAW_CMD_FULLSIZE (FD_RAW_CMD_SIZE + 1 + FD_RAW_REPLY_SIZE)
+
+	/* The command may take up the space initially intended for the reply
+	 * and the reply count. Needed for long 82078 commands such as RESTORE,
+	 * which takes 17 command bytes.
+	 */
+
 	unsigned char cmd_count;
-	unsigned char cmd[16];
+	unsigned char cmd[FD_RAW_CMD_SIZE];
 	unsigned char reply_count;
-	unsigned char reply[16];
+	unsigned char reply[FD_RAW_REPLY_SIZE];
 	int track;
 	int resultcode;
 
-- 
2.25.3

[PATCH v3 4/4] floppy: suppress UBSAN warning in setup_rw_floppy()

[Denis Efremov]

UBSAN: array-index-out-of-bounds in drivers/block/floppy.c:1521:45
index 16 is out of range for type 'unsigned char [16]'
Call Trace:
...
 setup_rw_floppy+0x5c3/0x7f0
 floppy_ready+0x2be/0x13b0
 process_one_work+0x2c1/0x5d0
 worker_thread+0x56/0x5e0
 kthread+0x122/0x170
 ret_from_fork+0x35/0x40

From include/uapi/linux/fd.h:
struct floppy_raw_cmd {
	...
	unsigned char cmd_count;
	unsigned char cmd[16];
	unsigned char reply_count;
	unsigned char reply[16];
	...
}

This out-of-bounds access is intentional. The command in struct
floppy_raw_cmd may take up the space initially intended for the reply
and the reply count. It is needed for long 82078 commands such as
RESTORE, which takes 17 command bytes. Initial cmd size is not enough
and since struct setup_rw_floppy is a part of uapi we check that
cmd_count is in [0:16+1+16] in raw_cmd_copyin().

The patch adds union with original cmd,reply_count,reply fields and
fullcmd field of equivalent size. The cmd accesses are turned to
fullcmd where appropriate to suppress UBSAN warning.

Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Denis Efremov <efremov@linux.com>
---
 drivers/block/floppy.c  |  4 ++--
 include/uapi/linux/fd.h | 11 ++++++++---
 2 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c
index 9e098d53b046..064c1acb9f00 100644
--- a/drivers/block/floppy.c
+++ b/drivers/block/floppy.c
@@ -1070,7 +1070,7 @@ static void setup_DMA(void)
 	if (raw_cmd->length == 0) {
 		print_hex_dump(KERN_INFO, "zero dma transfer size: ",
 			       DUMP_PREFIX_NONE, 16, 1,
-			       raw_cmd->cmd, raw_cmd->cmd_count, false);
+			       raw_cmd->fullcmd, raw_cmd->cmd_count, false);
 		cont->done(0);
 		fdc_state[current_fdc].reset = 1;
 		return;
@@ -1515,7 +1515,7 @@ static void setup_rw_floppy(void)
 
 	r = 0;
 	for (i = 0; i < raw_cmd->cmd_count; i++)
-		r |= output_byte(current_fdc, raw_cmd->cmd[i]);
+		r |= output_byte(current_fdc, raw_cmd->fullcmd[i]);
 
 	debugt(__func__, "rw_command");
 
diff --git a/include/uapi/linux/fd.h b/include/uapi/linux/fd.h
index 2e9c2c1c18e6..8b80c63b971c 100644
--- a/include/uapi/linux/fd.h
+++ b/include/uapi/linux/fd.h
@@ -371,9 +371,14 @@ struct floppy_raw_cmd {
 	 */
 
 	unsigned char cmd_count;
-	unsigned char cmd[FD_RAW_CMD_SIZE];
-	unsigned char reply_count;
-	unsigned char reply[FD_RAW_REPLY_SIZE];
+	union {
+		struct {
+			unsigned char cmd[FD_RAW_CMD_SIZE];
+			unsigned char reply_count;
+			unsigned char reply[FD_RAW_REPLY_SIZE];
+		};
+		unsigned char fullcmd[FD_RAW_CMD_FULLSIZE];
+	};
 	int track;
 	int resultcode;
 
-- 
2.25.3

Re: [PATCH v3 0/4] floppy: suppress UBSAN warning in setup_rw_floppy()

[Joe Perches]

On Fri, 2020-05-01 at 16:44 +0300, Denis Efremov wrote:
> These patches are based on Willy's cleanup patches
> https://lkml.org/lkml/2020/3/31/609.

Maybe add pr_fmt and clean up a few messages so
all the logging output is prefixed too.

---
 drivers/block/floppy.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c
index c3daa64..b26bb1 100644
--- a/drivers/block/floppy.c
+++ b/drivers/block/floppy.c
@@ -145,6 +145,8 @@
  * Better audit of register_blkdev.
  */
 
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
 #undef  FLOPPY_SILENT_DCL_CLEAR
 
 #define REALLY_SLOW_IO
@@ -1724,7 +1726,7 @@ irqreturn_t floppy_interrupt(int irq, void *dev_id)
 	if (current_fdc >= N_FDC || fdc_state[current_fdc].address == -1) {
 		/* we don't even know which FDC is the culprit */
 		pr_info("DOR0=%x\n", fdc_state[0].dor);
-		pr_info("floppy interrupt on bizarre fdc %d\n", current_fdc);
+		pr_info("interrupt on bizarre fdc %d\n", current_fdc);
 		pr_info("handler=%ps\n", handler);
 		is_alive(__func__, "bizarre fdc");
 		return IRQ_NONE;
@@ -2276,7 +2278,7 @@ static void request_done(int uptodate)
 	reschedule_timeout(MAXTIMEOUT, msg);
 
 	if (!req) {
-		pr_info("floppy.c: no request in request_done\n");
+		pr_info("no request in request_done\n");
 		return;
 	}
 
@@ -4181,8 +4183,7 @@ static void floppy_rb0_cb(struct bio *bio)
 	int drive = cbdata->drive;
 
 	if (bio->bi_status) {
-		pr_info("floppy: error %d while reading block 0\n",
-			bio->bi_status);
+		pr_info("error %d while reading block 0\n", bio->bi_status);
 		set_bit(FD_OPEN_SHOULD_FAIL_BIT, &drive_state[drive].flags);
 	}
 	complete(&cbdata->complete);
@@ -4954,7 +4955,7 @@ static void floppy_release_irq_and_dma(void)
 #endif
 
 	if (delayed_work_pending(&fd_timeout))
-		pr_info("floppy timer still active:%s\n", timeout_message);
+		pr_info("timer still active:%s\n", timeout_message);
 	if (delayed_work_pending(&fd_timer))
 		pr_info("auxiliary floppy timer still active\n");
 	if (work_pending(&floppy_work))

Re: [PATCH v3 0/4] floppy: suppress UBSAN warning in setup_rw_floppy()

[Denis Efremov]

On 5/1/20 9:02 PM, Joe Perches wrote:
> On Fri, 2020-05-01 at 16:44 +0300, Denis Efremov wrote:
>> These patches are based on Willy's cleanup patches
>> https://lkml.org/lkml/2020/3/31/609.
> 
> Maybe add pr_fmt and clean up a few messages so
> all the logging output is prefixed too.
>

Yes, I'm preparing next patchset with almost the same pr_fmt patch in it.
However, simply adding pr_fmt is not enough. We need to remove DPRINT macro
from the driver because it uses current_drive and this is wrong after Willy's cleanups.

#define DPRINT(format, args...) \                                                
        pr_info("floppy%d: " format, current_drive, ##args)

I also don't like debug_dcl and how it is used.
 
> ---
>  drivers/block/floppy.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c
> index c3daa64..b26bb1 100644
> --- a/drivers/block/floppy.c
> +++ b/drivers/block/floppy.c
> @@ -145,6 +145,8 @@
>   * Better audit of register_blkdev.
>   */
>  
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
>  #undef  FLOPPY_SILENT_DCL_CLEAR
>  
>  #define REALLY_SLOW_IO
> @@ -1724,7 +1726,7 @@ irqreturn_t floppy_interrupt(int irq, void *dev_id)
>  	if (current_fdc >= N_FDC || fdc_state[current_fdc].address == -1) {
>  		/* we don't even know which FDC is the culprit */
>  		pr_info("DOR0=%x\n", fdc_state[0].dor);
> -		pr_info("floppy interrupt on bizarre fdc %d\n", current_fdc);
> +		pr_info("interrupt on bizarre fdc %d\n", current_fdc);
>  		pr_info("handler=%ps\n", handler);
>  		is_alive(__func__, "bizarre fdc");
>  		return IRQ_NONE;
> @@ -2276,7 +2278,7 @@ static void request_done(int uptodate)
>  	reschedule_timeout(MAXTIMEOUT, msg);
>  
>  	if (!req) {
> -		pr_info("floppy.c: no request in request_done\n");
> +		pr_info("no request in request_done\n");
>  		return;
>  	}
>  
> @@ -4181,8 +4183,7 @@ static void floppy_rb0_cb(struct bio *bio)
>  	int drive = cbdata->drive;
>  
>  	if (bio->bi_status) {
> -		pr_info("floppy: error %d while reading block 0\n",
> -			bio->bi_status);
> +		pr_info("error %d while reading block 0\n", bio->bi_status);
>  		set_bit(FD_OPEN_SHOULD_FAIL_BIT, &drive_state[drive].flags);
>  	}
>  	complete(&cbdata->complete);
> @@ -4954,7 +4955,7 @@ static void floppy_release_irq_and_dma(void)
>  #endif
>  
>  	if (delayed_work_pending(&fd_timeout))
> -		pr_info("floppy timer still active:%s\n", timeout_message);
> +		pr_info("timer still active:%s\n", timeout_message);
>  	if (delayed_work_pending(&fd_timer))
>  		pr_info("auxiliary floppy timer still active\n");
>  	if (work_pending(&floppy_work))
> 
> 

Re: [PATCH v3 0/4] floppy: suppress UBSAN warning in setup_rw_floppy()

[Joe Perches]

On Fri, 2020-05-01 at 21:22 +0300, Denis Efremov wrote:
> On 5/1/20 9:02 PM, Joe Perches wrote:
> > On Fri, 2020-05-01 at 16:44 +0300, Denis Efremov wrote:
> > > These patches are based on Willy's cleanup patches
> > > https://lkml.org/lkml/2020/3/31/609.
> > 
> > Maybe add pr_fmt and clean up a few messages so
> > all the logging output is prefixed too.
> > 
> 
> Yes, I'm preparing next patchset with almost the same pr_fmt patch in it.
> However, simply adding pr_fmt is not enough. We need to remove DPRINT macro
> from the driver because it uses current_drive and this is wrong after Willy's cleanups.
> 
> #define DPRINT(format, args...) \                                                
>         pr_info("floppy%d: " format, current_drive, ##args)
> 
> I also don't like debug_dcl and how it is used.

great, thanks.

Re: [PATCH v3 0/4] floppy: suppress UBSAN warning in setup_rw_floppy()

[Denis Efremov]

On 5/1/20 4:44 PM, Denis Efremov wrote
> 
> The first patch removes pr_cont() in setup_DMA() and prints the contents of
> cmd buffer with print_hex_dump(). The last patch also touches these lines
> and changes cmd buffer to fullcmd. The 2,3 patches introduce defines to
> make it more clear why cmd_count in struct floppy_raw_cmd allows
> out-of-bounds access for cmd, reply_count, reply fields. Last patch
> handles the warning.

Applied,

https://github.com/evdenis/linux-floppy/tree/cleanups

Denis