| Message ID | 20200421125722.58959-4-efremov@linux.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | floppy: suppress UBSAN warning in setup_rw_floppy() | expand |
On Tue, Apr 21, 2020 at 03:57:22PM +0300, Denis Efremov wrote: > 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 replaces array subscript with pointer arithetic to suppress > UBSAN warning. Urghh. I think the better way would be to use a union that creates a larger cmd field, or something like: struct floppy_raw_cmd { ... u8 buf[34]; #define BUF_CMD_COUNT 0 #define BUF_CMD 1 #define BUF_REPLY_COUNT 17 #define BUF_REPLY 18 and use addressing based on that.
On Wed, Apr 22, 2020 at 12:09:21AM -0700, Christoph Hellwig wrote: > On Tue, Apr 21, 2020 at 03:57:22PM +0300, Denis Efremov wrote: > > 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 replaces array subscript with pointer arithetic to suppress > > UBSAN warning. > > Urghh. I think the better way would be to use a union that creates > a larger cmd field, or something like: > > struct floppy_raw_cmd { > ... > u8 buf[34]; > > #define BUF_CMD_COUNT 0 > #define BUF_CMD 1 > #define BUF_REPLY_COUNT 17 > #define BUF_REPLY 18 > > and use addressing based on that. But isn't it a problem if struct floppy_raw_cmd is exposed to uapi ? That said I remember a discussion with Linus who said that most if not all of the floppy parts leaking to uapi were more of a side effect of the include files reordering than a deliberate decision to expose it. So maybe that could remain the best solution indeed. I must say I don't feel very comfortable either with replacing p[i] with *(p+i) given that they are all supposed to be interchangeable and equivalent (as well as i[p] as strange as it can look). So we're not really protected against a later mechanical change or cleanup that reintroduces it :-/ Willy
On 4/22/20 10:17 AM, Willy Tarreau wrote: > On Wed, Apr 22, 2020 at 12:09:21AM -0700, Christoph Hellwig wrote: >> On Tue, Apr 21, 2020 at 03:57:22PM +0300, Denis Efremov wrote: >>> 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 replaces array subscript with pointer arithetic to suppress >>> UBSAN warning. >> > > But isn't it a problem if struct floppy_raw_cmd is exposed to uapi ? > That said I remember a discussion with Linus who said that most if not > all of the floppy parts leaking to uapi were more of a side effect of > the include files reordering than a deliberate decision to expose it. > So maybe that could remain the best solution indeed. struct floppy_raw_cmd is input/output structure for FDRAWCMD ioctl. > > I must say I don't feel very comfortable either with replacing p[i] > with *(p+i) given that they are all supposed to be interchangeable and > equivalent (as well as i[p] as strange as it can look). So we're not > really protected against a later mechanical change or cleanup that > reintroduces it :-/ >> Urghh. I think the better way would be to use a union that creates >> a larger cmd field, or something like: >> >> struct floppy_raw_cmd { >> ... >> u8 buf[34]; >> >> #define BUF_CMD_COUNT 0 >> #define BUF_CMD 1 >> #define BUF_REPLY_COUNT 17 >> #define BUF_REPLY 18 >> >> and use addressing based on that. What do you think about changing it this way? struct floppy_raw_cmd { unsigned char rate; -#define FD_RAW_CMD_SIZE 16 +#define FD_RAW_CMD_SIZE 33 #define FD_RAW_REPLY_SIZE 16 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 reserved[16]; + unsigned char reply_count; + unsigned char reply[FD_RAW_REPLY_SIZE]; + }; + unsigned char cmd[FD_RAW_CMD_SIZE]; + }; int track; Denis
On Wed, Apr 22, 2020 at 11:20:23AM +0300, Denis Efremov wrote: > What do you think about changing it this way? > > struct floppy_raw_cmd { > > unsigned char rate; > > -#define FD_RAW_CMD_SIZE 16 > +#define FD_RAW_CMD_SIZE 33 > #define FD_RAW_REPLY_SIZE 16 > > 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 reserved[16]; > + unsigned char reply_count; > + unsigned char reply[FD_RAW_REPLY_SIZE]; > + }; > + unsigned char cmd[FD_RAW_CMD_SIZE]; > + }; I don't think we can just change FD_RAW_CMD_SIZE or cmd as that could break userspace. But otherwise, yes something very much like that: > #define FD_RAW_CMD_SIZE 16 > #define FD_RAW_REPLY_SIZE 16 > +#define FD_RAW_FULL_CMD_SIZE (FD_RAW_CMD_SIZE + 1 + FD_RAW_CMD_SIZE) > > 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 full_cmd[FD_RAW_FULL_CMD_SIZE]; > + }; > int track; > > Denis ---end quoted text---
On 4/22/20 11:24 AM, Christoph Hellwig wrote: > > I don't think we can just change FD_RAW_CMD_SIZE or cmd as that could > break userspace. The second patch adds these defines FD_RAW_CMD_SIZE, FD_RAW_REPLY_SIZE. Currently they are not in the uapi. Ok, I will send v2. But otherwise, yes something very much like that: > >> #define FD_RAW_CMD_SIZE 16 >> #define FD_RAW_REPLY_SIZE 16 >> +#define FD_RAW_FULL_CMD_SIZE (FD_RAW_CMD_SIZE + 1 + FD_RAW_CMD_SIZE) >> >> 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 full_cmd[FD_RAW_FULL_CMD_SIZE]; >> + }; > >> int track; >> >> Denis > ---end quoted text--- >
diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c index 2169df796d18..63981b1f8d4c 100644 --- a/drivers/block/floppy.c +++ b/drivers/block/floppy.c @@ -1518,7 +1518,10 @@ 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]); + /* The command may take up the space initially intended for the + * reply and the reply count. + */ + r |= output_byte(current_fdc, *(raw_cmd->cmd + i)); debugt(__func__, "rw_command");
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 replaces array subscript with pointer arithetic to suppress UBSAN warning. Signed-off-by: Denis Efremov <efremov@linux.com> --- drivers/block/floppy.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)