dmu: fix integer overflow in dmu_read

[perfinion]

The params are uint64_t, but the offsets are calculated as 32bit ints.
PaX's Size Overflow catches this when formatting a zvol. This patch
changes them to also be uint64_t so there isnt an overflow.

Gentoo bug: #546490

PAX: offset: 1ffffb000 db->db_offset: 1ffffa000 db->db_size: 2000 size: 5000
PAX: size overflow detected in function dmu_read /var/tmp/portage/sys-fs/zfs-kmod-0.6.3-r1/work/zfs-zfs-0.6.3/module/zfs/../../module/zfs/dmu.c:781 cicus.366_146 max, count: 15
CPU: 1 PID: 2236 Comm: zvol/10 Tainted: P O 3.17.7-hardened-r1 #1
Call Trace:
[] ? dsl_dataset_get_holds+0x9d58/0x343ce [zfs]
[] dump_stack+0x4e/0x7a
[] ? dsl_dataset_get_holds+0x1aa9a/0x343ce [zfs]
[] report_size_overflow+0x36/0x40
[] dmu_read+0x52b/0x920 [zfs]
[] zrl_is_locked+0x7d1/0x1ce0 [zfs]
[] zil_clean+0x9d2/0xc00 [zfs]
[] zil_commit+0x21/0x30 [zfs]
[] zrl_is_locked+0xce1/0x1ce0 [zfs]
[] ? __schedule+0x547/0xbc0
[] taskq_cancel_id+0x2a6/0x5b0 [spl]
[] ? wake_up_state+0x20/0x20
[] ? taskq_cancel_id+0x110/0x5b0 [spl]
[] kthread+0xc4/0xe0
[] ? kthread_create_on_node+0x170/0x170
[] ret_from_fork+0x74/0xa0
[] ? kthread_create_on_node+0x170/0x170

Signed-off-by: Jason Zaman [email protected]
Reviewed-by: Emese Revfy [email protected]

[ryao]

This looks good to me.

[thegreatgazoo]

There seemed to be similar overflows also in dmu_read_req(), dmu_write_req(), dmu_read_uio(), and dmu_write_uio_dnode().

[avg-I]

Given that those values are offsets and sizes within a block it seems that using int is safe in practice.
Not sure if multi-gigabyte sized blocks are going to make sense any time soon.

[perfinion]

Given that those values are offsets and sizes within a block it seems that using int is safe in practice.
Not sure if multi-gigabyte sized blocks are going to make sense any time soon.

This cant be true. PaX's size overflow plugin uses double size and recalculates to compare if there is an overflow and only then triggers. and the numbers passed in when i hit it are bigger than a 32bit int. I hit it with a ZVOL tho so maybe for zvols its not limited to within a block?
regardless, changing them to (u)int64_t is better since the parameters to the method are also 64bit.

more about pax's size overflow is here: https://forums.grsecurity.net/viewtopic.php?f=7&t=3043

[behlendorf]

Treating these values all as 64-bit is definitely for the best since it makes everything explicit. @perfinion could you update this patch to made the same fix in dmu_write(), dmu_read_req(), dmu_write_req(), dmu_read_uio(), dmu_write_uio(), and dmu_write_uio_dnode() as @thegreatgazoo suggested.

[perfinion]

@behlendorf dmu_read() uses bcopy, all the other ones (dmu_write, dmu_read_req, dmu_req_copy etc) use memcopy. should I also swap dmu_read() to use memcopy to be consistent?

[behlendorf]

Yes, consistently is good. And under the covers bcopy will map to memcpy anyway.

[perfinion]

I pushed a new version, can you take a look?

• functions with fixed types: dmu_read, dmu_write, dmu_read_req, dmu_write_req, dmu_read_uio, dmu_write_uid_dnode.
• dmu_write_uio() does not have the same issue.
• dmu_read changed bcopy() to memcpy().

I booted this patch on my laptop and it builds and works. But I did not to any extensive testing other than the original dmu_read that i triggered.

[behlendorf]

@perfinion thanks for fixing up the additional cases. This LGTM and the two test failures were for unrelated issues. I'll get this merged tomorrow to give @thegreatgazoo and @ryao a chance to comment.