* [PATCH] iommu/vt-d: Fix shift overflow in qi_flush_dev_iotlb
@ 2017-12-12 22:43 Alex Williamson
2017-12-13 7:13 ` Peter Xu
2017-12-15 3:12 ` kbuild test robot
0 siblings, 2 replies; 8+ messages in thread
From: Alex Williamson @ 2017-12-12 22:43 UTC (permalink / raw)
To: iommu, alex.williamson; +Cc: linux-kernel, ashok.raj, tursulin, dwmw2
> Detected by ubsan:
>
> UBSAN: Undefined behaviour in drivers/iommu/dmar.c:1345:3
> shift exponent 64 is too large for 32-bit type 'int'
> CPU: 2 PID: 1167 Comm: perf_pmu Not tainted 4.14.0-rc5+ #532
> Hardware name: LENOVO 80MX/Lenovo E31-80, BIOS DCCN34WW(V2.03) 12/01/2015
> Call Trace:
> <IRQ>
> dump_stack+0xab/0xfe
> ? _atomic_dec_and_lock+0x112/0x112
> ? get_unsigned_val+0x48/0x91
> ubsan_epilogue+0x9/0x49
> __ubsan_handle_shift_out_of_bounds+0x1ea/0x241
> ? __ubsan_handle_load_invalid_value+0x136/0x136
> ? _raw_spin_unlock_irqrestore+0x32/0x50
> ? qi_submit_sync+0x642/0x820
> ? qi_flush_dev_iotlb+0x158/0x1b0
> qi_flush_dev_iotlb+0x158/0x1b0
> ? qi_flush_iotlb+0x110/0x110
> ? do_raw_spin_lock+0x93/0x130
> iommu_flush_dev_iotlb+0xff/0x170
> iommu_flush_iova+0x168/0x220
> iova_domain_flush+0x2b/0x50
> fq_flush_timeout+0xa6/0x1e0
> ? fq_ring_free+0x260/0x260
> ? call_timer_fn+0xfd/0x600
> call_timer_fn+0x160/0x600
> ? fq_ring_free+0x260/0x260
> ? trace_timer_cancel+0x1d0/0x1d0
> ? _raw_spin_unlock_irq+0x29/0x40
> ? fq_ring_free+0x260/0x260
> ? fq_ring_free+0x260/0x260
> run_timer_softirq+0x3bb/0x9a0
> ? timer_fixup_init+0x30/0x30
> ? __lock_is_held+0x35/0x150
> ? sched_clock_cpu+0x14/0x180
> __do_softirq+0x159/0x9c7
> irq_exit+0x118/0x170
> smp_apic_timer_interrupt+0xda/0x530
> apic_timer_interrupt+0x9d/0xb0
> </IRQ>
> RIP: 0010:__asan_load4+0xa/0x80
> RSP: 0018:ffff88012f647380 EFLAGS: 00000246 ORIG_RAX: ffffffffffffff10
> RAX: ffff7fffffffffff RBX: ffff88012f647548 RCX: ffffffff9a07ea01
> RDX: dffffc0000000000 RSI: ffff88012f647808 RDI: ffff88012f647548
> RBP: ffff88012f640000 R08: fffffbfff3b27e96 R09: fffffbfff3b27e95
> R10: ffffffff9d93f4ad R11: fffffbfff3b27e96 R12: ffff88012f648000
> R13: ffff88012f647558 R14: ffff88012f647550 R15: ffff88012f647808
> ? stack_access_ok+0x61/0x110
> stack_access_ok+0x6d/0x110
> deref_stack_reg+0x64/0x120
> ? __read_once_size_nocheck.constprop.3+0x50/0x50
> ? deref_stack_reg+0x120/0x120
> ? __kmalloc+0x13a/0x550
> unwind_next_frame+0xc04/0x14e0
> ? kasan_kmalloc+0xa0/0xd0
> ? __kmalloc+0x13a/0x550
> ? __kmalloc+0x13a/0x550
> ? deref_stack_reg+0x120/0x120
> ? unwind_next_frame+0xf3e/0x14e0
> ? i915_gem_do_execbuffer+0x4fd/0x2570 [i915]
> __save_stack_trace+0x7a/0x120
> ? __kmalloc+0x13a/0x550
> save_stack+0x33/0xa0
> ? save_stack+0x33/0xa0
> ? kasan_kmalloc+0xa0/0xd0
> ? _raw_spin_unlock+0x24/0x30
> ? deactivate_slab+0x650/0xb90
> ? ___slab_alloc+0x3e0/0x940
> ? ___slab_alloc+0x3e0/0x940
> ? i915_gem_do_execbuffer+0x4fd/0x2570 [i915]
> ? __lock_is_held+0x35/0x150
> ? mark_held_locks+0x33/0x130
> ? kasan_unpoison_shadow+0x30/0x40
> kasan_kmalloc+0xa0/0xd0
> __kmalloc+0x13a/0x550
> ? depot_save_stack+0x16a/0x7f0
> i915_gem_do_execbuffer+0x4fd/0x2570 [i915]
> ? save_stack+0x92/0xa0
> ? eb_relocate_slow+0x890/0x890 [i915]
> ? debug_check_no_locks_freed+0x200/0x200
> ? ___slab_alloc+0x3e0/0x940
> ? ___slab_alloc+0x3e0/0x940
> ? i915_gem_execbuffer2+0xdb/0x5f0 [i915]
> ? __lock_is_held+0x35/0x150
> ? i915_gem_execbuffer+0x580/0x580 [i915]
> i915_gem_execbuffer2+0x2c1/0x5f0 [i915]
> ? i915_gem_execbuffer+0x580/0x580 [i915]
> ? lock_downgrade+0x310/0x310
> ? i915_gem_execbuffer+0x580/0x580 [i915]
> drm_ioctl_kernel+0xdc/0x190
> drm_ioctl+0x46a/0x6e0
> ? i915_gem_execbuffer+0x580/0x580 [i915]
> ? drm_setversion+0x430/0x430
> ? lock_downgrade+0x310/0x310
> do_vfs_ioctl+0x138/0xbf0
> ? ioctl_preallocate+0x180/0x180
> ? do_raw_spin_unlock+0xf5/0x1d0
> ? do_raw_spin_trylock+0x90/0x90
> ? task_work_run+0x35/0x120
> ? mark_held_locks+0x33/0x130
> ? _raw_spin_unlock_irq+0x29/0x40
> ? mark_held_locks+0x33/0x130
> ? entry_SYSCALL_64_fastpath+0x5/0xad
> ? trace_hardirqs_on_caller+0x184/0x360
> SyS_ioctl+0x3b/0x70
> entry_SYSCALL_64_fastpath+0x18/0xad
> RIP: 0033:0x7fc0e1f0d587
> RSP: 002b:00007ffcfd9929f8 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
> RAX: ffffffffffffffda RBX: 0000000000000006 RCX: 00007fc0e1f0d587
> RDX: 00007ffcfd992a90 RSI: 0000000040406469 RDI: 0000000000000003
> RBP: 0000000000000003 R08: 00007ffcfd992a50 R09: 0000000000000000
> R10: 0000000000000073 R11: 0000000000000246 R12: 0000000000000001
> R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
>
> Code is:
>
> void qi_flush_dev_iotlb(struct intel_iommu *iommu, u16 sid, u16 qdep,
> u64 addr, unsigned mask)
> {
> struct qi_desc desc;
>
> if (mask) {
> BUG_ON(addr & ((1 << (VTD_PAGE_SHIFT + mask)) - 1));
>
> ^^^ This last line is where the warning comes. Digging deeper
> VTD_PAGE_SHIFT is 12 and the passed in mask comes from iommu_flush_iova
> (via iommu_flush_dev_iotlb) as MAX_AGAW_PFN_WIDTH which is:
>
> #define MAX_AGAW_WIDTH 64
> #define MAX_AGAW_PFN_WIDTH (MAX_AGAW_WIDTH - VTD_PAGE_SHIFT)
>
> So mask is 64, which overflows the left shift in the BUG_ON.
The resulting shift is 64, mask itself is (64 - 12). I assume the code
is expecting the overflow to result in zero, so we assert that addr is
64bit aligned, ie. zero.
Reported-by: Tvrtko Ursulin <tursulin@ursulin.net>
Link: https://lkml.org/lkml/2017/10/20/752
Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
drivers/iommu/dmar.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/iommu/dmar.c b/drivers/iommu/dmar.c
index 9a7ffd13c7f0..87888b102057 100644
--- a/drivers/iommu/dmar.c
+++ b/drivers/iommu/dmar.c
@@ -1345,7 +1345,9 @@ void qi_flush_dev_iotlb(struct intel_iommu *iommu, u16 sid, u16 qdep,
struct qi_desc desc;
if (mask) {
- BUG_ON(addr & ((1 << (VTD_PAGE_SHIFT + mask)) - 1));
+ BUG_ON((mask > MAX_AGAW_PFN_WIDTH) ||
+ ((mask == MAX_AGAW_PFN_WIDTH) && addr) ||
+ (addr & ((1 << (VTD_PAGE_SHIFT + mask)) - 1)));
addr |= (1ULL << (VTD_PAGE_SHIFT + mask - 1)) - 1;
desc.high = QI_DEV_IOTLB_ADDR(addr) | QI_DEV_IOTLB_SIZE;
} else
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] iommu/vt-d: Fix shift overflow in qi_flush_dev_iotlb
2017-12-12 22:43 [PATCH] iommu/vt-d: Fix shift overflow in qi_flush_dev_iotlb Alex Williamson
@ 2017-12-13 7:13 ` Peter Xu
2017-12-13 15:58 ` Alex Williamson
2017-12-15 3:12 ` kbuild test robot
1 sibling, 1 reply; 8+ messages in thread
From: Peter Xu @ 2017-12-13 7:13 UTC (permalink / raw)
To: Alex Williamson; +Cc: iommu, tursulin, dwmw2, linux-kernel
On Tue, Dec 12, 2017 at 03:43:08PM -0700, Alex Williamson wrote:
[...]
> diff --git a/drivers/iommu/dmar.c b/drivers/iommu/dmar.c
> index 9a7ffd13c7f0..87888b102057 100644
> --- a/drivers/iommu/dmar.c
> +++ b/drivers/iommu/dmar.c
> @@ -1345,7 +1345,9 @@ void qi_flush_dev_iotlb(struct intel_iommu *iommu, u16 sid, u16 qdep,
> struct qi_desc desc;
>
> if (mask) {
> - BUG_ON(addr & ((1 << (VTD_PAGE_SHIFT + mask)) - 1));
> + BUG_ON((mask > MAX_AGAW_PFN_WIDTH) ||
> + ((mask == MAX_AGAW_PFN_WIDTH) && addr) ||
> + (addr & ((1 << (VTD_PAGE_SHIFT + mask)) - 1)));
Could it work if we just use 1ULL instead of 1 here? Thanks,
--
Peter Xu
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] iommu/vt-d: Fix shift overflow in qi_flush_dev_iotlb
2017-12-13 7:13 ` Peter Xu
@ 2017-12-13 15:58 ` Alex Williamson
2017-12-13 16:41 ` Hook, Gary
0 siblings, 1 reply; 8+ messages in thread
From: Alex Williamson @ 2017-12-13 15:58 UTC (permalink / raw)
To: Peter Xu; +Cc: iommu, tursulin, dwmw2, linux-kernel
On Wed, 13 Dec 2017 15:13:55 +0800
Peter Xu <peterx@redhat.com> wrote:
> On Tue, Dec 12, 2017 at 03:43:08PM -0700, Alex Williamson wrote:
>
> [...]
>
> > diff --git a/drivers/iommu/dmar.c b/drivers/iommu/dmar.c
> > index 9a7ffd13c7f0..87888b102057 100644
> > --- a/drivers/iommu/dmar.c
> > +++ b/drivers/iommu/dmar.c
> > @@ -1345,7 +1345,9 @@ void qi_flush_dev_iotlb(struct intel_iommu *iommu, u16 sid, u16 qdep,
> > struct qi_desc desc;
> >
> > if (mask) {
> > - BUG_ON(addr & ((1 << (VTD_PAGE_SHIFT + mask)) - 1));
> > + BUG_ON((mask > MAX_AGAW_PFN_WIDTH) ||
> > + ((mask == MAX_AGAW_PFN_WIDTH) && addr) ||
> > + (addr & ((1 << (VTD_PAGE_SHIFT + mask)) - 1)));
>
> Could it work if we just use 1ULL instead of 1 here? Thanks,
In either case we're talking about shifting off the end of the
variable, which I understand to be undefined. Right? Thanks,
Alex
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] iommu/vt-d: Fix shift overflow in qi_flush_dev_iotlb
2017-12-13 15:58 ` Alex Williamson
@ 2017-12-13 16:41 ` Hook, Gary
2017-12-13 17:15 ` Alex Williamson
0 siblings, 1 reply; 8+ messages in thread
From: Hook, Gary @ 2017-12-13 16:41 UTC (permalink / raw)
To: Alex Williamson, Peter Xu; +Cc: iommu, dwmw2, linux-kernel, tursulin
On 12/13/2017 9:58 AM, Alex Williamson wrote:
> On Wed, 13 Dec 2017 15:13:55 +0800
> Peter Xu <peterx@redhat.com> wrote:
>
>> On Tue, Dec 12, 2017 at 03:43:08PM -0700, Alex Williamson wrote:
>>
>> [...]
>>
>>> diff --git a/drivers/iommu/dmar.c b/drivers/iommu/dmar.c
>>> index 9a7ffd13c7f0..87888b102057 100644
>>> --- a/drivers/iommu/dmar.c
>>> +++ b/drivers/iommu/dmar.c
>>> @@ -1345,7 +1345,9 @@ void qi_flush_dev_iotlb(struct intel_iommu *iommu, u16 sid, u16 qdep,
>>> struct qi_desc desc;
>>>
>>> if (mask) {
>>> - BUG_ON(addr & ((1 << (VTD_PAGE_SHIFT + mask)) - 1));
>>> + BUG_ON((mask > MAX_AGAW_PFN_WIDTH) ||
>>> + ((mask == MAX_AGAW_PFN_WIDTH) && addr) ||
>>> + (addr & ((1 << (VTD_PAGE_SHIFT + mask)) - 1)));
>>
>> Could it work if we just use 1ULL instead of 1 here? Thanks,
>
> In either case we're talking about shifting off the end of the
> variable, which I understand to be undefined. Right? Thanks,
How so? Bits fall off the left (MSB) end, zeroes fill in the right (LSB)
end. I believe that behavior is pretty set.
The only problem here is that the promotion of integral types is (at the
very least) unclear in this statement. As for the proposal, do we know
that 1ULL is always going to be the same size as addr?
I would suggest, as pedantic as it sounds, creating a local u64
variable, initialized to 1, and using that in the BUG_ON:
u64 ubit = 1;
...
if (mask) {
BUG_ON(addr & ((ubit << (VTD_PAGE_SHIFT + mask)) - 1));
I believe this forces everything to be appropriately wide, and the same
size?
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] iommu/vt-d: Fix shift overflow in qi_flush_dev_iotlb
2017-12-13 16:41 ` Hook, Gary
@ 2017-12-13 17:15 ` Alex Williamson
2017-12-13 17:31 ` Hook, Gary
0 siblings, 1 reply; 8+ messages in thread
From: Alex Williamson @ 2017-12-13 17:15 UTC (permalink / raw)
To: Hook, Gary; +Cc: Peter Xu, iommu, dwmw2, linux-kernel, tursulin
On Wed, 13 Dec 2017 10:41:47 -0600
"Hook, Gary" <ghook@amd.com> wrote:
> On 12/13/2017 9:58 AM, Alex Williamson wrote:
> > On Wed, 13 Dec 2017 15:13:55 +0800
> > Peter Xu <peterx@redhat.com> wrote:
> >
> >> On Tue, Dec 12, 2017 at 03:43:08PM -0700, Alex Williamson wrote:
> >>
> >> [...]
> >>
> >>> diff --git a/drivers/iommu/dmar.c b/drivers/iommu/dmar.c
> >>> index 9a7ffd13c7f0..87888b102057 100644
> >>> --- a/drivers/iommu/dmar.c
> >>> +++ b/drivers/iommu/dmar.c
> >>> @@ -1345,7 +1345,9 @@ void qi_flush_dev_iotlb(struct intel_iommu *iommu, u16 sid, u16 qdep,
> >>> struct qi_desc desc;
> >>>
> >>> if (mask) {
> >>> - BUG_ON(addr & ((1 << (VTD_PAGE_SHIFT + mask)) - 1));
> >>> + BUG_ON((mask > MAX_AGAW_PFN_WIDTH) ||
> >>> + ((mask == MAX_AGAW_PFN_WIDTH) && addr) ||
> >>> + (addr & ((1 << (VTD_PAGE_SHIFT + mask)) - 1)));
> >>
> >> Could it work if we just use 1ULL instead of 1 here? Thanks,
> >
> > In either case we're talking about shifting off the end of the
> > variable, which I understand to be undefined. Right? Thanks,
>
> How so? Bits fall off the left (MSB) end, zeroes fill in the right (LSB)
> end. I believe that behavior is pretty set.
Maybe I'm relying too much on stackoverflow, but:
https://stackoverflow.com/questions/11270492/what-does-the-c-standard-say-about-bitshifting-more-bits-than-the-width-of-type
Thanks,
Alex
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] iommu/vt-d: Fix shift overflow in qi_flush_dev_iotlb
2017-12-13 17:15 ` Alex Williamson
@ 2017-12-13 17:31 ` Hook, Gary
2017-12-15 7:29 ` Peter Xu
0 siblings, 1 reply; 8+ messages in thread
From: Hook, Gary @ 2017-12-13 17:31 UTC (permalink / raw)
To: Alex Williamson; +Cc: Peter Xu, iommu, dwmw2, linux-kernel, tursulin
On 12/13/2017 11:15 AM, Alex Williamson wrote:
> On Wed, 13 Dec 2017 10:41:47 -0600
> "Hook, Gary" <ghook@amd.com> wrote:
>
>> On 12/13/2017 9:58 AM, Alex Williamson wrote:
>>> On Wed, 13 Dec 2017 15:13:55 +0800
>>> Peter Xu <peterx@redhat.com> wrote:
>>>
>>>> On Tue, Dec 12, 2017 at 03:43:08PM -0700, Alex Williamson wrote:
>>>>
>>>> [...]
>>>>
>>>>> diff --git a/drivers/iommu/dmar.c b/drivers/iommu/dmar.c
>>>>> index 9a7ffd13c7f0..87888b102057 100644
>>>>> --- a/drivers/iommu/dmar.c
>>>>> +++ b/drivers/iommu/dmar.c
>>>>> @@ -1345,7 +1345,9 @@ void qi_flush_dev_iotlb(struct intel_iommu *iommu, u16 sid, u16 qdep,
>>>>> struct qi_desc desc;
>>>>>
>>>>> if (mask) {
>>>>> - BUG_ON(addr & ((1 << (VTD_PAGE_SHIFT + mask)) - 1));
>>>>> + BUG_ON((mask > MAX_AGAW_PFN_WIDTH) ||
>>>>> + ((mask == MAX_AGAW_PFN_WIDTH) && addr) ||
>>>>> + (addr & ((1 << (VTD_PAGE_SHIFT + mask)) - 1)));
>>>>
>>>> Could it work if we just use 1ULL instead of 1 here? Thanks,
>>>
>>> In either case we're talking about shifting off the end of the
>>> variable, which I understand to be undefined. Right? Thanks,
>>
>> How so? Bits fall off the left (MSB) end, zeroes fill in the right (LSB)
>> end. I believe that behavior is pretty set.
>
> Maybe I'm relying too much on stackoverflow, but:
>
> https://stackoverflow.com/questions/11270492/what-does-the-c-standard-say-about-bitshifting-more-bits-than-the-width-of-type
No, probably not. I don't have my copy of c99 handy, so can't check it.
But it is beyond me why any compiler implementation would choose to use
a rotate instead of a shift... probably a performance issue.
So, yeah, when you have silly parameters, you get what you get.
I'll stick to my suggestion. Which seems unambiguous... but I could be
wrong.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] iommu/vt-d: Fix shift overflow in qi_flush_dev_iotlb
2017-12-12 22:43 [PATCH] iommu/vt-d: Fix shift overflow in qi_flush_dev_iotlb Alex Williamson
2017-12-13 7:13 ` Peter Xu
@ 2017-12-15 3:12 ` kbuild test robot
1 sibling, 0 replies; 8+ messages in thread
From: kbuild test robot @ 2017-12-15 3:12 UTC (permalink / raw)
To: Alex Williamson
Cc: kbuild-all, iommu, alex.williamson, linux-kernel, ashok.raj,
tursulin, dwmw2
[-- Attachment #1: Type: text/plain, Size: 2951 bytes --]
Hi Alex,
I love your patch! Yet something to improve:
[auto build test ERROR on v4.15-rc3]
[also build test ERROR on next-20171214]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Alex-Williamson/iommu-vt-d-Fix-shift-overflow-in-qi_flush_dev_iotlb/20171215-094227
config: x86_64-randconfig-x000-201750 (attached as .config)
compiler: gcc-7 (Debian 7.2.0-12) 7.2.1 20171025
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All error/warnings (new ones prefixed by >>):
In file included from include/linux/string.h:6:0,
from include/uapi/linux/uuid.h:22,
from include/linux/uuid.h:19,
from include/linux/mod_devicetable.h:13,
from include/linux/pci.h:21,
from drivers//iommu/dmar.c:31:
drivers//iommu/dmar.c: In function 'qi_flush_dev_iotlb':
>> drivers//iommu/dmar.c:1348:18: error: 'MAX_AGAW_PFN_WIDTH' undeclared (first use in this function)
BUG_ON((mask > MAX_AGAW_PFN_WIDTH) ||
^
include/linux/compiler.h:77:42: note: in definition of macro 'unlikely'
# define unlikely(x) __builtin_expect(!!(x), 0)
^
>> drivers//iommu/dmar.c:1348:3: note: in expansion of macro 'BUG_ON'
BUG_ON((mask > MAX_AGAW_PFN_WIDTH) ||
^~~~~~
drivers//iommu/dmar.c:1348:18: note: each undeclared identifier is reported only once for each function it appears in
BUG_ON((mask > MAX_AGAW_PFN_WIDTH) ||
^
include/linux/compiler.h:77:42: note: in definition of macro 'unlikely'
# define unlikely(x) __builtin_expect(!!(x), 0)
^
>> drivers//iommu/dmar.c:1348:3: note: in expansion of macro 'BUG_ON'
BUG_ON((mask > MAX_AGAW_PFN_WIDTH) ||
^~~~~~
vim +/MAX_AGAW_PFN_WIDTH +1348 drivers//iommu/dmar.c
1341
1342 void qi_flush_dev_iotlb(struct intel_iommu *iommu, u16 sid, u16 qdep,
1343 u64 addr, unsigned mask)
1344 {
1345 struct qi_desc desc;
1346
1347 if (mask) {
> 1348 BUG_ON((mask > MAX_AGAW_PFN_WIDTH) ||
1349 ((mask == MAX_AGAW_PFN_WIDTH) && addr) ||
1350 (addr & ((1 << (VTD_PAGE_SHIFT + mask)) - 1)));
1351 addr |= (1ULL << (VTD_PAGE_SHIFT + mask - 1)) - 1;
1352 desc.high = QI_DEV_IOTLB_ADDR(addr) | QI_DEV_IOTLB_SIZE;
1353 } else
1354 desc.high = QI_DEV_IOTLB_ADDR(addr);
1355
1356 if (qdep >= QI_DEV_IOTLB_MAX_INVS)
1357 qdep = 0;
1358
1359 desc.low = QI_DEV_IOTLB_SID(sid) | QI_DEV_IOTLB_QDEP(qdep) |
1360 QI_DIOTLB_TYPE;
1361
1362 qi_submit_sync(&desc, iommu);
1363 }
1364
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 27927 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] iommu/vt-d: Fix shift overflow in qi_flush_dev_iotlb
2017-12-13 17:31 ` Hook, Gary
@ 2017-12-15 7:29 ` Peter Xu
0 siblings, 0 replies; 8+ messages in thread
From: Peter Xu @ 2017-12-15 7:29 UTC (permalink / raw)
To: Hook, Gary; +Cc: Alex Williamson, iommu, dwmw2, linux-kernel, tursulin
On Wed, Dec 13, 2017 at 11:31:02AM -0600, Hook, Gary wrote:
> On 12/13/2017 11:15 AM, Alex Williamson wrote:
> > On Wed, 13 Dec 2017 10:41:47 -0600
> > "Hook, Gary" <ghook@amd.com> wrote:
> >
> > > On 12/13/2017 9:58 AM, Alex Williamson wrote:
> > > > On Wed, 13 Dec 2017 15:13:55 +0800
> > > > Peter Xu <peterx@redhat.com> wrote:
> > > > > On Tue, Dec 12, 2017 at 03:43:08PM -0700, Alex Williamson wrote:
> > > > >
> > > > > [...]
> > > > > > diff --git a/drivers/iommu/dmar.c b/drivers/iommu/dmar.c
> > > > > > index 9a7ffd13c7f0..87888b102057 100644
> > > > > > --- a/drivers/iommu/dmar.c
> > > > > > +++ b/drivers/iommu/dmar.c
> > > > > > @@ -1345,7 +1345,9 @@ void qi_flush_dev_iotlb(struct intel_iommu *iommu, u16 sid, u16 qdep,
> > > > > > struct qi_desc desc;
> > > > > > if (mask) {
> > > > > > - BUG_ON(addr & ((1 << (VTD_PAGE_SHIFT + mask)) - 1));
> > > > > > + BUG_ON((mask > MAX_AGAW_PFN_WIDTH) ||
> > > > > > + ((mask == MAX_AGAW_PFN_WIDTH) && addr) ||
> > > > > > + (addr & ((1 << (VTD_PAGE_SHIFT + mask)) - 1)));
> > > > >
> > > > > Could it work if we just use 1ULL instead of 1 here? Thanks,
> > > >
> > > > In either case we're talking about shifting off the end of the
> > > > variable, which I understand to be undefined. Right? Thanks,
> > >
> > > How so? Bits fall off the left (MSB) end, zeroes fill in the right (LSB)
> > > end. I believe that behavior is pretty set.
> >
> > Maybe I'm relying too much on stackoverflow, but:
> >
> > https://stackoverflow.com/questions/11270492/what-does-the-c-standard-say-about-bitshifting-more-bits-than-the-width-of-type
>
> No, probably not. I don't have my copy of c99 handy, so can't check it. But
> it is beyond me why any compiler implementation would choose to use a rotate
> instead of a shift... probably a performance issue.
>
> So, yeah, when you have silly parameters, you get what you get.
>
> I'll stick to my suggestion. Which seems unambiguous... but I could be
> wrong.
Hi, Alex, Hook,
I did a quick test:
xz-mi:tmp $ cat a.c
#include <stdio.h>
void main(void)
{
unsigned long long val = 0x8000000000000001ULL;
int shift;
printf("origin: 0x%llx\n", val);
shift = 1; printf("shl 1: 0x%llx\n", val << shift);
shift = 62; printf("shl 62: 0x%llx\n", val << shift);
shift = 63; printf("shl 63: 0x%llx\n", val << shift);
shift = 64; printf("shl 64: 0x%llx\n", val << shift);
shift = 65; printf("shl 65: 0x%llx\n", val << shift);
}
xz-mi:tmp $ gcc -std=c99 a.c
xz-mi:tmp $ ./a.out
origin: 0x8000000000000001
shl 1: 0x2
shl 62: 0x4000000000000000
shl 63: 0x8000000000000000
shl 64: 0x8000000000000001
shl 65: 0x2
xz-mi:tmp $ objdump -d a.out | grep -A20 "<main>"
00000000004004d7 <main>:
4004d7: 55 push %rbp
4004d8: 48 89 e5 mov %rsp,%rbp
4004db: 48 83 ec 10 sub $0x10,%rsp
4004df: 48 b8 01 00 00 00 00 movabs $0x8000000000000001,%rax
4004e6: 00 00 80
4004e9: 48 89 45 f8 mov %rax,-0x8(%rbp)
4004ed: 48 8b 45 f8 mov -0x8(%rbp),%rax
4004f1: 48 89 c6 mov %rax,%rsi
4004f4: bf 60 06 40 00 mov $0x400660,%edi
4004f9: b8 00 00 00 00 mov $0x0,%eax
4004fe: e8 ed fe ff ff callq 4003f0 <printf@plt>
400503: c7 45 f4 01 00 00 00 movl $0x1,-0xc(%rbp)
40050a: 8b 45 f4 mov -0xc(%rbp),%eax
40050d: 48 8b 55 f8 mov -0x8(%rbp),%rdx
400511: 89 c1 mov %eax,%ecx
400513: 48 d3 e2 shl %cl,%rdx
400516: 48 89 d0 mov %rdx,%rax
400519: 48 89 c6 mov %rax,%rsi
40051c: bf 70 06 40 00 mov $0x400670,%edi
400521: b8 00 00 00 00 mov $0x0,%eax
So it seems not really a rotation operation, but it looks more like
convering a "shifting N" into "shifting N%64" when N>=64. So now I
agree with Alex's change. Thanks all.
--
Peter Xu
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2017-12-15 7:30 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-12-12 22:43 [PATCH] iommu/vt-d: Fix shift overflow in qi_flush_dev_iotlb Alex Williamson
2017-12-13 7:13 ` Peter Xu
2017-12-13 15:58 ` Alex Williamson
2017-12-13 16:41 ` Hook, Gary
2017-12-13 17:15 ` Alex Williamson
2017-12-13 17:31 ` Hook, Gary
2017-12-15 7:29 ` Peter Xu
2017-12-15 3:12 ` kbuild test robot
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox