| Message ID | 1458562256-2882-1-git-send-email-nicstange@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Mar 21, 2016 at 8:10 AM, Nicolai Stange <nicstange@gmail.com> wrote: > The values of all but the RADEON_HPD_NONE members of the radeon_hpd_id > enum transform 1:1 into bit positions within the 'enabled' bitset as > assembled by evergreen_hpd_init(): > > enabled |= 1 << radeon_connector->hpd.hpd; > > However, if ->hpd.hpd happens to equal RADEON_HPD_NONE == 0xff, UBSAN > reports > > UBSAN: Undefined behaviour in drivers/gpu/drm/radeon/evergreen.c:1867:16 > shift exponent 255 is too large for 32-bit type 'int' > [...] > Call Trace: > [<ffffffff818c4d35>] dump_stack+0xbc/0x117 > [<ffffffff818c4c79>] ? _atomic_dec_and_lock+0x169/0x169 > [<ffffffff819411bb>] ubsan_epilogue+0xd/0x4e > [<ffffffff81941cbc>] __ubsan_handle_shift_out_of_bounds+0x1fb/0x254 > [<ffffffffa0ba7f2e>] ? atom_execute_table+0x3e/0x50 [radeon] > [<ffffffff81941ac1>] ? __ubsan_handle_load_invalid_value+0x158/0x158 > [<ffffffffa0b87700>] ? radeon_get_pll_use_mask+0x130/0x130 [radeon] > [<ffffffff81219930>] ? wake_up_klogd_work_func+0x60/0x60 > [<ffffffff8121a35e>] ? vprintk_default+0x3e/0x60 > [<ffffffffa0c603c4>] evergreen_hpd_init+0x274/0x2d0 [radeon] > [<ffffffffa0c603c4>] ? evergreen_hpd_init+0x274/0x2d0 [radeon] > [<ffffffffa0bd196e>] radeon_modeset_init+0x8ce/0x18d0 [radeon] > [<ffffffffa0b71d86>] radeon_driver_load_kms+0x186/0x350 [radeon] > [<ffffffffa03b6b16>] drm_dev_register+0xc6/0x100 [drm] > [<ffffffffa03bc8c4>] drm_get_pci_dev+0xe4/0x490 [drm] > [<ffffffff814b83f0>] ? kfree+0x220/0x370 > [<ffffffffa0b687c2>] radeon_pci_probe+0x112/0x140 [radeon] > [...] > ===================================================================== > radeon 0000:01:00.0: No connectors reported connected with modes > > At least on x86, there should be no user-visible impact as there > > 1 << 0xff == 1 << (0xff & 31) == 1 << 31 > > holds and 31 > RADEON_MAX_HPD_PINS. Thus, this patch is a cosmetic one. > > All of the above applies analogously to evergreen_hpd_fini(). > > Silence UBSAN by checking ->hpd.hpd for RADEON_HPD_NONE before oring it > into the 'enabled' bitset in evergreen_hpd_init() or the 'disabled' bitset > in evergreen_hpd_fini() respectively. > > Signed-off-by: Nicolai Stange <nicstange@gmail.com> Can you please fix all instances of this? This same pattern is repeated in various asic families. Alex > --- > Applicable to linux-next-20160321. > > Changes to v1: > - Turn commit message's impact part into a non-impact part. > > drivers/gpu/drm/radeon/evergreen.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/radeon/evergreen.c b/drivers/gpu/drm/radeon/evergreen.c > index 76c4bdf..6360717 100644 > --- a/drivers/gpu/drm/radeon/evergreen.c > +++ b/drivers/gpu/drm/radeon/evergreen.c > @@ -1864,7 +1864,8 @@ void evergreen_hpd_init(struct radeon_device *rdev) > break; > } > radeon_hpd_set_polarity(rdev, radeon_connector->hpd.hpd); > - enabled |= 1 << radeon_connector->hpd.hpd; > + if (radeon_connector->hpd.hpd != RADEON_HPD_NONE) > + enabled |= 1 << radeon_connector->hpd.hpd; > } > radeon_irq_kms_enable_hpd(rdev, enabled); > } > @@ -1907,7 +1908,8 @@ void evergreen_hpd_fini(struct radeon_device *rdev) > default: > break; > } > - disabled |= 1 << radeon_connector->hpd.hpd; > + if (radeon_connector->hpd.hpd != RADEON_HPD_NONE) > + disabled |= 1 << radeon_connector->hpd.hpd; > } > radeon_irq_kms_disable_hpd(rdev, disabled); > } > -- > 2.7.3 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
Alex Deucher <alexdeucher@gmail.com> writes: > On Mon, Mar 21, 2016 at 8:10 AM, Nicolai Stange <nicstange@gmail.com> wrote: >> The values of all but the RADEON_HPD_NONE members of the radeon_hpd_id >> enum transform 1:1 into bit positions within the 'enabled' bitset as >> assembled by evergreen_hpd_init(): >> >> enabled |= 1 << radeon_connector->hpd.hpd; >> >> However, if ->hpd.hpd happens to equal RADEON_HPD_NONE == 0xff, UBSAN >> reports >> >> UBSAN: Undefined behaviour in drivers/gpu/drm/radeon/evergreen.c:1867:16 >> shift exponent 255 is too large for 32-bit type 'int' >> [...] >> Call Trace: >> [<ffffffff818c4d35>] dump_stack+0xbc/0x117 >> [<ffffffff818c4c79>] ? _atomic_dec_and_lock+0x169/0x169 >> [<ffffffff819411bb>] ubsan_epilogue+0xd/0x4e >> [<ffffffff81941cbc>] __ubsan_handle_shift_out_of_bounds+0x1fb/0x254 >> [<ffffffffa0ba7f2e>] ? atom_execute_table+0x3e/0x50 [radeon] >> [<ffffffff81941ac1>] ? __ubsan_handle_load_invalid_value+0x158/0x158 >> [<ffffffffa0b87700>] ? radeon_get_pll_use_mask+0x130/0x130 [radeon] >> [<ffffffff81219930>] ? wake_up_klogd_work_func+0x60/0x60 >> [<ffffffff8121a35e>] ? vprintk_default+0x3e/0x60 >> [<ffffffffa0c603c4>] evergreen_hpd_init+0x274/0x2d0 [radeon] >> [<ffffffffa0c603c4>] ? evergreen_hpd_init+0x274/0x2d0 [radeon] >> [<ffffffffa0bd196e>] radeon_modeset_init+0x8ce/0x18d0 [radeon] >> [<ffffffffa0b71d86>] radeon_driver_load_kms+0x186/0x350 [radeon] >> [<ffffffffa03b6b16>] drm_dev_register+0xc6/0x100 [drm] >> [<ffffffffa03bc8c4>] drm_get_pci_dev+0xe4/0x490 [drm] >> [<ffffffff814b83f0>] ? kfree+0x220/0x370 >> [<ffffffffa0b687c2>] radeon_pci_probe+0x112/0x140 [radeon] >> [...] >> ===================================================================== >> radeon 0000:01:00.0: No connectors reported connected with modes >> >> At least on x86, there should be no user-visible impact as there >> >> 1 << 0xff == 1 << (0xff & 31) == 1 << 31 >> >> holds and 31 > RADEON_MAX_HPD_PINS. Thus, this patch is a cosmetic one. >> >> All of the above applies analogously to evergreen_hpd_fini(). >> >> Silence UBSAN by checking ->hpd.hpd for RADEON_HPD_NONE before oring it >> into the 'enabled' bitset in evergreen_hpd_init() or the 'disabled' bitset >> in evergreen_hpd_fini() respectively. >> >> Signed-off-by: Nicolai Stange <nicstange@gmail.com> > > Can you please fix all instances of this? This same pattern is > repeated in various asic families. Sure. The places to fix are: $ git grep -n '1 *<<.*hpd' -- drivers/gpu/drm/radeon/ drivers/gpu/drm/radeon/evergreen.c:1868: enabled |= 1 << radeon_connector->hpd.hpd; drivers/gpu/drm/radeon/evergreen.c:1912: disabled |= 1 << radeon_connector->hpd.hpd; drivers/gpu/drm/radeon/r100.c:595: enable |= 1 << radeon_connector->hpd.hpd; drivers/gpu/drm/radeon/r100.c:617: disable |= 1 << radeon_connector->hpd.hpd; drivers/gpu/drm/radeon/r600.c:1005: enable |= 1 << radeon_connector->hpd.hpd; drivers/gpu/drm/radeon/r600.c:1058: disable |= 1 << radeon_connector->hpd.hpd; drivers/gpu/drm/radeon/rs600.c:416: enable |= 1 << radeon_connector->hpd.hpd; drivers/gpu/drm/radeon/rs600.c:442: disable |= 1 << radeon_connector->hpd.hpd; May I throw all of those into one single patch? Thanks, Nicolai > >> --- >> Applicable to linux-next-20160321. >> >> Changes to v1: >> - Turn commit message's impact part into a non-impact part. >> >> drivers/gpu/drm/radeon/evergreen.c | 6 ++++-- >> 1 file changed, 4 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/radeon/evergreen.c b/drivers/gpu/drm/radeon/evergreen.c >> index 76c4bdf..6360717 100644 >> --- a/drivers/gpu/drm/radeon/evergreen.c >> +++ b/drivers/gpu/drm/radeon/evergreen.c >> @@ -1864,7 +1864,8 @@ void evergreen_hpd_init(struct radeon_device *rdev) >> break; >> } >> radeon_hpd_set_polarity(rdev, radeon_connector->hpd.hpd); >> - enabled |= 1 << radeon_connector->hpd.hpd; >> + if (radeon_connector->hpd.hpd != RADEON_HPD_NONE) >> + enabled |= 1 << radeon_connector->hpd.hpd; >> } >> radeon_irq_kms_enable_hpd(rdev, enabled); >> } >> @@ -1907,7 +1908,8 @@ void evergreen_hpd_fini(struct radeon_device *rdev) >> default: >> break; >> } >> - disabled |= 1 << radeon_connector->hpd.hpd; >> + if (radeon_connector->hpd.hpd != RADEON_HPD_NONE) >> + disabled |= 1 << radeon_connector->hpd.hpd; >> } >> radeon_irq_kms_disable_hpd(rdev, disabled); >> } >> -- >> 2.7.3 >> >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Tue, Mar 22, 2016 at 8:29 AM, Nicolai Stange <nicstange@gmail.com> wrote: > Alex Deucher <alexdeucher@gmail.com> writes: > >> On Mon, Mar 21, 2016 at 8:10 AM, Nicolai Stange <nicstange@gmail.com> wrote: >>> The values of all but the RADEON_HPD_NONE members of the radeon_hpd_id >>> enum transform 1:1 into bit positions within the 'enabled' bitset as >>> assembled by evergreen_hpd_init(): >>> >>> enabled |= 1 << radeon_connector->hpd.hpd; >>> >>> However, if ->hpd.hpd happens to equal RADEON_HPD_NONE == 0xff, UBSAN >>> reports >>> >>> UBSAN: Undefined behaviour in drivers/gpu/drm/radeon/evergreen.c:1867:16 >>> shift exponent 255 is too large for 32-bit type 'int' >>> [...] >>> Call Trace: >>> [<ffffffff818c4d35>] dump_stack+0xbc/0x117 >>> [<ffffffff818c4c79>] ? _atomic_dec_and_lock+0x169/0x169 >>> [<ffffffff819411bb>] ubsan_epilogue+0xd/0x4e >>> [<ffffffff81941cbc>] __ubsan_handle_shift_out_of_bounds+0x1fb/0x254 >>> [<ffffffffa0ba7f2e>] ? atom_execute_table+0x3e/0x50 [radeon] >>> [<ffffffff81941ac1>] ? __ubsan_handle_load_invalid_value+0x158/0x158 >>> [<ffffffffa0b87700>] ? radeon_get_pll_use_mask+0x130/0x130 [radeon] >>> [<ffffffff81219930>] ? wake_up_klogd_work_func+0x60/0x60 >>> [<ffffffff8121a35e>] ? vprintk_default+0x3e/0x60 >>> [<ffffffffa0c603c4>] evergreen_hpd_init+0x274/0x2d0 [radeon] >>> [<ffffffffa0c603c4>] ? evergreen_hpd_init+0x274/0x2d0 [radeon] >>> [<ffffffffa0bd196e>] radeon_modeset_init+0x8ce/0x18d0 [radeon] >>> [<ffffffffa0b71d86>] radeon_driver_load_kms+0x186/0x350 [radeon] >>> [<ffffffffa03b6b16>] drm_dev_register+0xc6/0x100 [drm] >>> [<ffffffffa03bc8c4>] drm_get_pci_dev+0xe4/0x490 [drm] >>> [<ffffffff814b83f0>] ? kfree+0x220/0x370 >>> [<ffffffffa0b687c2>] radeon_pci_probe+0x112/0x140 [radeon] >>> [...] >>> ===================================================================== >>> radeon 0000:01:00.0: No connectors reported connected with modes >>> >>> At least on x86, there should be no user-visible impact as there >>> >>> 1 << 0xff == 1 << (0xff & 31) == 1 << 31 >>> >>> holds and 31 > RADEON_MAX_HPD_PINS. Thus, this patch is a cosmetic one. >>> >>> All of the above applies analogously to evergreen_hpd_fini(). >>> >>> Silence UBSAN by checking ->hpd.hpd for RADEON_HPD_NONE before oring it >>> into the 'enabled' bitset in evergreen_hpd_init() or the 'disabled' bitset >>> in evergreen_hpd_fini() respectively. >>> >>> Signed-off-by: Nicolai Stange <nicstange@gmail.com> >> >> Can you please fix all instances of this? This same pattern is >> repeated in various asic families. > > Sure. > > The places to fix are: > $ git grep -n '1 *<<.*hpd' -- drivers/gpu/drm/radeon/ > drivers/gpu/drm/radeon/evergreen.c:1868: enabled |= 1 << radeon_connector->hpd.hpd; > drivers/gpu/drm/radeon/evergreen.c:1912: disabled |= 1 << radeon_connector->hpd.hpd; > drivers/gpu/drm/radeon/r100.c:595: enable |= 1 << radeon_connector->hpd.hpd; > drivers/gpu/drm/radeon/r100.c:617: disable |= 1 << radeon_connector->hpd.hpd; > drivers/gpu/drm/radeon/r600.c:1005: enable |= 1 << radeon_connector->hpd.hpd; > drivers/gpu/drm/radeon/r600.c:1058: disable |= 1 << radeon_connector->hpd.hpd; > drivers/gpu/drm/radeon/rs600.c:416: enable |= 1 << radeon_connector->hpd.hpd; > drivers/gpu/drm/radeon/rs600.c:442: disable |= 1 << radeon_connector->hpd.hpd; > > May I throw all of those into one single patch? Yes, that would be fine. Alex > > > Thanks, > > Nicolai > > >> >>> --- >>> Applicable to linux-next-20160321. >>> >>> Changes to v1: >>> - Turn commit message's impact part into a non-impact part. >>> >>> drivers/gpu/drm/radeon/evergreen.c | 6 ++++-- >>> 1 file changed, 4 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/radeon/evergreen.c b/drivers/gpu/drm/radeon/evergreen.c >>> index 76c4bdf..6360717 100644 >>> --- a/drivers/gpu/drm/radeon/evergreen.c >>> +++ b/drivers/gpu/drm/radeon/evergreen.c >>> @@ -1864,7 +1864,8 @@ void evergreen_hpd_init(struct radeon_device *rdev) >>> break; >>> } >>> radeon_hpd_set_polarity(rdev, radeon_connector->hpd.hpd); >>> - enabled |= 1 << radeon_connector->hpd.hpd; >>> + if (radeon_connector->hpd.hpd != RADEON_HPD_NONE) >>> + enabled |= 1 << radeon_connector->hpd.hpd; >>> } >>> radeon_irq_kms_enable_hpd(rdev, enabled); >>> } >>> @@ -1907,7 +1908,8 @@ void evergreen_hpd_fini(struct radeon_device *rdev) >>> default: >>> break; >>> } >>> - disabled |= 1 << radeon_connector->hpd.hpd; >>> + if (radeon_connector->hpd.hpd != RADEON_HPD_NONE) >>> + disabled |= 1 << radeon_connector->hpd.hpd; >>> } >>> radeon_irq_kms_disable_hpd(rdev, disabled); >>> } >>> -- >>> 2.7.3 >>> >>> _______________________________________________ >>> dri-devel mailing list >>> dri-devel@lists.freedesktop.org >>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
diff --git a/drivers/gpu/drm/radeon/evergreen.c b/drivers/gpu/drm/radeon/evergreen.c index 76c4bdf..6360717 100644 --- a/drivers/gpu/drm/radeon/evergreen.c +++ b/drivers/gpu/drm/radeon/evergreen.c @@ -1864,7 +1864,8 @@ void evergreen_hpd_init(struct radeon_device *rdev) break; } radeon_hpd_set_polarity(rdev, radeon_connector->hpd.hpd); - enabled |= 1 << radeon_connector->hpd.hpd; + if (radeon_connector->hpd.hpd != RADEON_HPD_NONE) + enabled |= 1 << radeon_connector->hpd.hpd; } radeon_irq_kms_enable_hpd(rdev, enabled); } @@ -1907,7 +1908,8 @@ void evergreen_hpd_fini(struct radeon_device *rdev) default: break; } - disabled |= 1 << radeon_connector->hpd.hpd; + if (radeon_connector->hpd.hpd != RADEON_HPD_NONE) + disabled |= 1 << radeon_connector->hpd.hpd; } radeon_irq_kms_disable_hpd(rdev, disabled); }
The values of all but the RADEON_HPD_NONE members of the radeon_hpd_id enum transform 1:1 into bit positions within the 'enabled' bitset as assembled by evergreen_hpd_init(): enabled |= 1 << radeon_connector->hpd.hpd; However, if ->hpd.hpd happens to equal RADEON_HPD_NONE == 0xff, UBSAN reports UBSAN: Undefined behaviour in drivers/gpu/drm/radeon/evergreen.c:1867:16 shift exponent 255 is too large for 32-bit type 'int' [...] Call Trace: [<ffffffff818c4d35>] dump_stack+0xbc/0x117 [<ffffffff818c4c79>] ? _atomic_dec_and_lock+0x169/0x169 [<ffffffff819411bb>] ubsan_epilogue+0xd/0x4e [<ffffffff81941cbc>] __ubsan_handle_shift_out_of_bounds+0x1fb/0x254 [<ffffffffa0ba7f2e>] ? atom_execute_table+0x3e/0x50 [radeon] [<ffffffff81941ac1>] ? __ubsan_handle_load_invalid_value+0x158/0x158 [<ffffffffa0b87700>] ? radeon_get_pll_use_mask+0x130/0x130 [radeon] [<ffffffff81219930>] ? wake_up_klogd_work_func+0x60/0x60 [<ffffffff8121a35e>] ? vprintk_default+0x3e/0x60 [<ffffffffa0c603c4>] evergreen_hpd_init+0x274/0x2d0 [radeon] [<ffffffffa0c603c4>] ? evergreen_hpd_init+0x274/0x2d0 [radeon] [<ffffffffa0bd196e>] radeon_modeset_init+0x8ce/0x18d0 [radeon] [<ffffffffa0b71d86>] radeon_driver_load_kms+0x186/0x350 [radeon] [<ffffffffa03b6b16>] drm_dev_register+0xc6/0x100 [drm] [<ffffffffa03bc8c4>] drm_get_pci_dev+0xe4/0x490 [drm] [<ffffffff814b83f0>] ? kfree+0x220/0x370 [<ffffffffa0b687c2>] radeon_pci_probe+0x112/0x140 [radeon] [...] ===================================================================== radeon 0000:01:00.0: No connectors reported connected with modes At least on x86, there should be no user-visible impact as there 1 << 0xff == 1 << (0xff & 31) == 1 << 31 holds and 31 > RADEON_MAX_HPD_PINS. Thus, this patch is a cosmetic one. All of the above applies analogously to evergreen_hpd_fini(). Silence UBSAN by checking ->hpd.hpd for RADEON_HPD_NONE before oring it into the 'enabled' bitset in evergreen_hpd_init() or the 'disabled' bitset in evergreen_hpd_fini() respectively. Signed-off-by: Nicolai Stange <nicstange@gmail.com> --- Applicable to linux-next-20160321. Changes to v1: - Turn commit message's impact part into a non-impact part. drivers/gpu/drm/radeon/evergreen.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)