Re: [PATCH] fs: Fix signed integer overflow for vfs_setpos

[Ding Tianhong <dingtianhong@huawei.com>]

On 2017/12/7 23:27, Al Viro wrote:
> On Thu, Dec 07, 2017 at 09:19:10PM +0800, Ding Tianhong wrote:
>> The undefined behaviour sanatizer detected an signed integer overflow like this:
>>
>> r0 = memfd_create(&(0x7f0000002000-0x12)="2e726571756573745f6b65795f6175746800",0x0)
>> lseek(r0, 0x4040000000000000, 0x1)
>> setsockopt$inet6_IPV6_FLOWLABEL_MGR(r0, 0x29, 0x20,
>> &(0x7f000000b000-0xd)={@empty={[0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0,
>> 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0]}, 0x9, 0x1, 0xff, 0x2, 0x6, 0x1,0xd27}, 0x20)
>> mmap(&(0x7f000000e000/0x1000)=nil, 0x1000, 0x3, 0x32,0xffffffffffffffff, 0x0)
>> ioctl$sock_SIOCGSKNS(r0, 0x894c, &(0x7f000000f000-0x4)=0x10000)
>> ---------------------------------------------------------------------------------
>> UBSAN: Undefined behaviour in fs/read_write.c:107:12
>> signed integer overflow:
>> 4629700416936869888 + 4629700416936869888 cannot be represented in type
>> 'long long int'
>> CPU: 0 PID: 11653 Comm: syz-executor0 Not tainted 4.x.xx+ #2
>> Hardware name: linux,dummy-virt (DT)
>> Call trace:
>> [<ffffffc00008f4d0>] dump_backtrace+0x0/0x2a0
>> [<ffffffc00008f790>] show_stack+0x20/0x30
>> [<ffffffc000ec3b5c>] dump_stack+0x11c/0x16c
>> [<ffffffc000ec3e80>] ubsan_epilogue+0x18/0x70
>> [<ffffffc000ec4ca0>] handle_overflow+0x14c/0x188
>> [<ffffffc000ec4d10>] __ubsan_handle_add_overflow+0x34/0x44
>> [<ffffffc000327740>] generic_file_llseek_size+0x1f8/0x2a0
>> [<ffffffc0002826fc>] shmem_file_llseek+0x7c/0x1f8
>> [<ffffffc000327b88>] SyS_lseek+0xc0/0x118
>> --------------------------------------------------------------------------------
>>
>> The problem happened because the calculation of signed integer resulted
>> an overflow for the signed integer, so use the unsigned integer to avoid
>> undefined behaviour when it does overflow.
> 
> TBH, I don't like that solution - there's too much of "make UBSAN STFU" in
> it.  Besides, there are very similar places elsewhere.  Right next to this
> one there's default_llseek(), with its
>                 case SEEK_CUR:
>                         if (offset == 0) {
>                                 retval = file->f_pos;
>                                 goto out;
>                         }
>                         offset += file->f_pos;
>                         break;
> and offset is loff_t there.  Exact same issue, IOW.  Grepping around shows
> tons of similar places.  E.g. ceph_llseek() has
>                 if (offset == 0) {
>                         ret = file->f_pos;
>                         goto out;
>                 }
>                 offset += file->f_pos;
>                 break;
> with offset being loff_t and ocfs2_file_llseek() is the same.  memory_lseek()
> does something very similar, except that it doesn't use vfs_setpos(),
> ditto for xillybus_llseek(), wil_pmc_llseek(), hmcdrv_dev_seek(), etc.
> 
> That kind of whack-a-mole ("UBSAN has stepped on that one, let's plug it",
> while the other places like that keep breeding) is, IMO, the wrong approach ;-/
> 
> BTW, a fun unrelated bogosity:
> static loff_t scom_llseek(struct file *file, loff_t offset, int whence)
> {
>         switch (whence) {
>         case SEEK_CUR:
>                 break;
>         case SEEK_SET:
>                 file->f_pos = offset;
>                 break;
>         default:
>                 return -EINVAL;
>         }
> 
>         return offset;
> }
> IOW, lseek(fd, SEEK_CUR, n) quietly returns n there.  Separate issue, though...
> 

Totally agree with you, this problem also make me very confused, but looks like
the undefined behaviour is really critical issue,  so should we reconsider this
problem and solve it completely ?

Thanks
Ding

> .
>