Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1752002AbcCUISB (ORCPT <rfc822;w@1wt.eu>);
	Mon, 21 Mar 2016 04:18:01 -0400
Received: from mail-wm0-f67.google.com ([74.125.82.67]:35860 "EHLO
	mail-wm0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1751943AbcCUIRq (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Mon, 21 Mar 2016 04:17:46 -0400
From: Nicolai Stange <nicstange@gmail.com>
To: Nicolai Stange <nicstange@gmail.com>
Cc: Alex Deucher <alexander.deucher@amd.com>,
        Christian =?utf-8?Q?K=C3=B6?= =?utf-8?Q?nig?= 
	<christian.koenig@amd.com>,
        David Airlie <airlied@linux.ie>, dri-devel@lists.freedesktop.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] drm/radeon: evergreen_hpd_init()/_fini(): fix HPD IRQ bitset
References: <1458417401-3756-1-git-send-email-nicstange@gmail.com>
Date: Mon, 21 Mar 2016 09:17:25 +0100
In-Reply-To: <1458417401-3756-1-git-send-email-nicstange@gmail.com> (Nicolai
	Stange's message of "Sat, 19 Mar 2016 20:56:41 +0100")
Message-ID: <87oaa82qju.fsf@gmail.com>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.0.92 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 3575
Lines: 89

Nicolai Stange <nicstange@gmail.com> writes:

> 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
>
> The net effect is that radeon_irq_kms_enable_hpd() enables the HPD
> interrupts for all HPD pins in the range from 0 to RADEON_MAX_HPD_PINS.


And this is obviously wrong: I mixed the shift count 0xff with the final
value when reasoning about the implications :(
1 << 0xff == 1 << 0x1f == 2^31 on x86 at least. Thus the net effect is
nothing.

I will resend later this day with an updated description.




> The system seems to work without any noticeable glitches though.
>
> 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-20160318.
>
>  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);
>  }