Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.36 as permitted sender) client-ip=23.128.96.36;
Message-ID: <716d82a7-146b-4b67-a434-dd7c32e77d57@amd.com>
Date:   Thu, 30 Nov 2023 07:41:40 +0100
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] drm/radeon/r600_cs: Fix possible int overflows in
 r600_cs_check_reg()
Content-Language: en-US
To:     Alex Deucher <alexdeucher@gmail.com>
Cc:     Nikita Zhandarovich <n.zhandarovich@fintech.ru>,
        Alex Deucher <alexander.deucher@amd.com>,
        "Pan, Xinhui" <Xinhui.Pan@amd.com>, linux-kernel@vger.kernel.org,
        dri-devel@lists.freedesktop.org, amd-gfx@lists.freedesktop.org
References: <20231129152230.7931-1-n.zhandarovich@fintech.ru>
 <6a26b0c9-cbea-4ca2-bc16-79ed53e3a6fe@amd.com>
 <CADnq5_P-Bxghq4N4GXZXUNwGR5BPyU2qQXDYzDeNkvn=VSTfwg@mail.gmail.com>
From:   =?UTF-8?Q?Christian_K=C3=B6nig?= <christian.koenig@amd.com>
In-Reply-To: <CADnq5_P-Bxghq4N4GXZXUNwGR5BPyU2qQXDYzDeNkvn=VSTfwg@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?UHdQcFFuRXRiZTlLMTlGZ1h6TU94a0FXSjBlMTFJYm9DcmdjZFZJMjBMRlRV?=
 =?utf-8?B?MlhKMVdvRG1SUGExRHhDOUpHaUFLUkY3ZkdrVENnVnNLTE11dEJYekhrdG5N?=
 =?utf-8?B?aWIxUXJqKzNvd01TVXZCRXFHYUoyU1R1VllmcU11V3RIZHU4QmI1WlhnR0xi?=
 =?utf-8?B?TlUvd2FySlBRUGRhZXhTd2JGVUNSMHhSOGNTSEpSOVlQZUplMGNMeW5GL3or?=
 =?utf-8?B?RUtGdFRVNHViaU01SUZBRHlYejV1bUI5TW5JMVpXbFFhUUl3RHFnaWtqY3hU?=
 =?utf-8?B?L0RrK1JCQmZyZDVYOHk4bTVZZHJrY2pqNzJtRldRVlo3Y3JxbFM3clNucU90?=
 =?utf-8?B?WmlVVEEvOU9hQzV5U2VLdUhWU1ROR1VoczEzaFVvUkNlZDA0Zit6T3B2TU9D?=
 =?utf-8?B?S3pWb1VGWVpmcnVqbzQ2QlFyakJpVHRubTdzRHZHYTB5dFlhQXJGZ0NWZ0hN?=
 =?utf-8?B?OGk5M2ZiZmJBM0FOVXJtejRxQWxOMWN3R0dNMDNuWWtrdklYSTM4U0JTQmN0?=
 =?utf-8?B?dnp0eFhDREVIL3lxaXpRbkczQmZpN3hHSFpKTWw5emtqVjd3Z0ZlSWprejlt?=
 =?utf-8?B?VGVjdVFCbjM3M2JJSU5SMnpJdDJPbDZqdUFNY0hqOElnNTNRSzdlMWcveGtK?=
 =?utf-8?B?czFyeTg2Qk81N1RwSXR0eWVqUDZ5OUxneVI4K1hIczlVSmdFYmxuc2ZDSnI2?=
 =?utf-8?B?akpxNDZuN2R2TElRbis2SFV5QWlrUjRSN2xvN2RISXBBekk1ZFpOZkw5cjhR?=
 =?utf-8?B?UGwyNERqZ3l1L1pNZ080bXJHTk1zbUpYMVJFdmV3alVGTDJOUFJtQjZsQ01L?=
 =?utf-8?B?WWxIRmUxUitDUmVCUGtJc3JWTGlLWGNESG40U1dQUjBaL1dlREZpcnNvNlBu?=
 =?utf-8?B?T0FnT25IQ1V3aFpBY09XcDlZSlplSC9uT0JnZ2hqWUUxZXd3ekEzZnd3dVVG?=
 =?utf-8?B?TTlnM2hUL2laUncyaU56WVNKOVQyK2RHZDdNR3p5ZGNZQUdRMlN0RkF0UVUv?=
 =?utf-8?B?c2FhYXdITjc3VUdFNFQ0SC9QZzFnSDVDVWNFdXhvdGo5WERZN0ZqOVI5bjc5?=
 =?utf-8?B?UXZnT2NlcmVFYlJ4b1Z2Ry9GNTZjajl0cTJ3UVd3WEVwcjVCTkpuY3dQRDlr?=
 =?utf-8?B?TnA4VVMrcXlDa1QyY3F2anlpTW5iN0dDTmhBRHQrbmE0ak95bkk5Ylh4ay9z?=
 =?utf-8?B?cWJkOXB5QjlWOEgreDh5cWt6a3RKa3k3dHgzTVN0WUhhUzNIaGRpMnBaOSty?=
 =?utf-8?B?RDZSK1dYdTlQQkg4TDRkZ2p3eFE0QzBNRDFmbEZNNXlBMy9ORzFrL2tkVXkx?=
 =?utf-8?B?UHBPUkJHZS9mZlFUdksza3dIdGY1alN4Mm9WWVZMaFdTT205WWdlQ1RmNW1w?=
 =?utf-8?B?dmVFekxEL2VmUzhReWhVRGMvYnd6MmdHWmxjemdWYTJIdUQ4UU5TZFphd3Fw?=
 =?utf-8?B?RHRUc3FXZ1cxQ3BzUE9wcUtrTm5tMTJFYVlTdVRDSk5wMHZNZ01DWkFaVlZR?=
 =?utf-8?B?Ti9ybzZkK3NmbjE0MUNwdllGTEFCbG80Sk1YMG9WSXNKVWtmUGNkcU8zVFhT?=
 =?utf-8?B?bjdPM0pkSlo5MmFBTkZoRUQ4SVIydWkrZERVNWJ4dnhLZEVOWnVYSWtoM0pD?=
 =?utf-8?B?QTd1cVhvMWhvR3ovMExRWXQ0SmcvMmdhN2tnOE40d1hlWVYwOFhMTEtvMGQ1?=
 =?utf-8?B?Y2xyRHZBaXpGanJEMGFYYkVUdFV3bDg1eGFqMlhxOGdBQzVBT0hURWd1M0J1?=
 =?utf-8?B?MjE3ZVJpRW5oWVMrVmlOU09MSkZNNk5Kcklqbmh5WGRNR2Zkc1gzbkZETDZZ?=
 =?utf-8?B?c3MyOFRsWnVndnJpMTB2R0wwYW0rSm1JQ1JtbFVabFJ3aGcyM1ZxMVBMSkQr?=
 =?utf-8?B?Z2dWa1JVMFAySDduem5FTjRWK1JyMGtHZG5yR2RCVGVod1B5N2dvdThtQTdq?=
 =?utf-8?B?MVZ4MDA5cHlaTmlab0drRmFzMU4zUjN1S0NRTGgwSW1jMHNuZXBkMCszK2tx?=
 =?utf-8?B?TWI1a0xmWVRudHY3NkRJYnp2N2lrcEphblhhN25vMk8rR3FsRVJRS3pIRGx0?=
 =?utf-8?B?c3B1ZU9hN3FtTTRhem9HcDFTeUFMeHJ5cGx1TXo4UHNYZ1JVTTdMTFliNVBY?=
 =?utf-8?Q?TYAU=3D?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: d95cce96-66ee-4fab-aefe-08dbf16f6adb
X-MS-Exchange-CrossTenant-AuthSource: BN8PR12MB3587.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 30 Nov 2023 06:41:44.1948
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: zjlJge+VMim3eGl0AZn5YaI3dH5lZyWwE5DrzZujxs0sEHIhSz7j6ZqGihPrpRji
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN9PR12MB5179
X-Spam-Status: No, score=-0.9 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED,
	DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE
	autolearn=unavailable autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on pete.vger.email
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (pete.vger.email [0.0.0.0]); Wed, 29 Nov 2023 22:42:13 -0800 (PST)

Am 29.11.23 um 17:03 schrieb Alex Deucher:
> On Wed, Nov 29, 2023 at 10:47 AM Christian König
> <christian.koenig@amd.com> wrote:
>> Am 29.11.23 um 16:22 schrieb Nikita Zhandarovich:
>>> While improbable, there may be a chance of hitting integer
>>> overflow when the result of radeon_get_ib_value() gets shifted
>>> left.
>>>
>>> Avoid it by casting one of the operands to larger data type (u64).
>>>
>>> Found by Linux Verification Center (linuxtesting.org) with static
>>> analysis tool SVACE.
>> Well IIRC cb_color_bo_offset is just 32bits anyway, so this doesn't
>> change anything.
> All of the GPU addresses in the structure are u64.  The registers are
> 32 bits which is why they are 256 byte aligned.  That said, I think
> the MC on the chips supported by this code are only 32 bits so we
> shouldn't see any addresses greater than 32 bits, but this seems like
> good to do from a coding perspective.  Otherwise, we'll keep getting
> this patch.

Just double checked it, in evergreen_cs_track the fields are just 
32bits, but in r600_cs_track they are 64bits.

No idea if we every used the full address space, but it's probably a 
good point to merge it just to silence the static checker warning.

Christian.

>
> Alex
>
>
> Alex
>
>> Regards,
>> Christian.
>>
>>> Fixes: 1729dd33d20b ("drm/radeon/kms: r600 CS parser fixes")
>>> Signed-off-by: Nikita Zhandarovich <n.zhandarovich@fintech.ru>
>>> ---
>>>    drivers/gpu/drm/radeon/r600_cs.c | 4 ++--
>>>    1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/radeon/r600_cs.c b/drivers/gpu/drm/radeon/r600_cs.c
>>> index 638f861af80f..6cf54a747749 100644
>>> --- a/drivers/gpu/drm/radeon/r600_cs.c
>>> +++ b/drivers/gpu/drm/radeon/r600_cs.c
>>> @@ -1275,7 +1275,7 @@ static int r600_cs_check_reg(struct radeon_cs_parser *p, u32 reg, u32 idx)
>>>                        return -EINVAL;
>>>                }
>>>                tmp = (reg - CB_COLOR0_BASE) / 4;
>>> -             track->cb_color_bo_offset[tmp] = radeon_get_ib_value(p, idx) << 8;
>>> +             track->cb_color_bo_offset[tmp] = (u64)radeon_get_ib_value(p, idx) << 8;
>>>                ib[idx] += (u32)((reloc->gpu_offset >> 8) & 0xffffffff);
>>>                track->cb_color_base_last[tmp] = ib[idx];
>>>                track->cb_color_bo[tmp] = reloc->robj;
>>> @@ -1302,7 +1302,7 @@ static int r600_cs_check_reg(struct radeon_cs_parser *p, u32 reg, u32 idx)
>>>                                        "0x%04X\n", reg);
>>>                        return -EINVAL;
>>>                }
>>> -             track->htile_offset = radeon_get_ib_value(p, idx) << 8;
>>> +             track->htile_offset = (u64)radeon_get_ib_value(p, idx) << 8;
>>>                ib[idx] += (u32)((reloc->gpu_offset >> 8) & 0xffffffff);
>>>                track->htile_bo = reloc->robj;
>>>                track->db_dirty = true;