| Message ID | 20200426130728.63399-3-efremov@linux.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | floppy: suppress UBSAN warning in setup_rw_floppy() | expand |
On Sun, 2020-04-26 at 16:07 +0300, Denis Efremov wrote: > 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. [] > diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c [] > @@ -1847,7 +1846,7 @@ static void show_floppy(int fdc) > output_log[(i + output_log_pos) % OLOGSIZE].jiffies); > pr_info("last result at %lu\n", resultjiffies); > pr_info("last redo_fd_request at %lu\n", lastredo); > - print_hex_dump(KERN_INFO, "", DUMP_PREFIX_NONE, 16, 1, > + print_hex_dump(KERN_INFO, "", DUMP_PREFIX_NONE, FD_RAW_REPLY_SIZE, 1, > reply_buffer, resultsize, true); FD_RAW_REPLY_SIZE happens to be 16, but it's misleading to use it here. This use of 16 is not for FD_RAW_REPLY_SIZE, but the width of the line being dumped, and this value must be either 16 or 32.
On 4/26/20 11:28 PM, Joe Perches wrote: > On Sun, 2020-04-26 at 16:07 +0300, Denis Efremov wrote: >> 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. > [] >> diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c > [] >> @@ -1847,7 +1846,7 @@ static void show_floppy(int fdc) >> output_log[(i + output_log_pos) % OLOGSIZE].jiffies); >> pr_info("last result at %lu\n", resultjiffies); >> pr_info("last redo_fd_request at %lu\n", lastredo); >> - print_hex_dump(KERN_INFO, "", DUMP_PREFIX_NONE, 16, 1, >> + print_hex_dump(KERN_INFO, "", DUMP_PREFIX_NONE, FD_RAW_REPLY_SIZE, 1, >> reply_buffer, resultsize, true); > > FD_RAW_REPLY_SIZE happens to be 16, but it's misleading > to use it here. > > This use of 16 is not for FD_RAW_REPLY_SIZE, but the > width of the line > being dumped, and this value must be > either 16 or 32. > Yes, you are right. Thanks for catching. I will resend the patches. Denis
Looks good modulo the hexdump nitpick:
Reviewed-by: Christoph Hellwig <hch@lst.de>
diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c index ac2023c757e3..052ba457956e 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 @@ -1165,7 +1164,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; @@ -1847,7 +1846,7 @@ static void show_floppy(int fdc) output_log[(i + output_log_pos) % OLOGSIZE].jiffies); pr_info("last result at %lu\n", resultjiffies); pr_info("last redo_fd_request at %lu\n", lastredo); - print_hex_dump(KERN_INFO, "", DUMP_PREFIX_NONE, 16, 1, + print_hex_dump(KERN_INFO, "", DUMP_PREFIX_NONE, FD_RAW_REPLY_SIZE, 1, reply_buffer, resultsize, true); pr_info("status=%x\n", fdc_inb(fdc, FD_STATUS)); @@ -3082,7 +3081,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]; @@ -3193,18 +3192,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;
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. Signed-off-by: Denis Efremov <efremov@linux.com> --- drivers/block/floppy.c | 21 ++++++--------------- include/uapi/linux/fd.h | 14 ++++++++++++-- 2 files changed, 18 insertions(+), 17 deletions(-)