linux-kernel - Re: [PATCH 3/3] floppy: suppress UBSAN warning in setup_rw

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <c2cc742d-23f9-ce03-c326-7df648161427@linux.com>
Date:   Wed, 22 Apr 2020 11:20:23 +0300
From:   Denis Efremov <efremov@...ux.com>
To:     Willy Tarreau <w@....eu>, Christoph Hellwig <hch@...radead.org>
Cc:     linux-block@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/3] floppy: suppress UBSAN warning in setup_rw_floppy()



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