Message-ID: <505a76a9-6110-3ddb-0f15-059b60922482@suse.de>
Date:   Thu, 9 Jan 2020 11:49:17 +0100
From:   Thomas Zimmermann <tzimmermann@...e.de>
To:     Christian König <christian.koenig@....com>,
        Alex Deucher <alexdeucher@...il.com>,
        Kees Cook <keescook@...omium.org>
Cc:     kernel-hardening@...ts.openwall.com,
        David Airlie <airlied@...ux.ie>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        LKML <linux-kernel@...r.kernel.org>,
        amd-gfx list <amd-gfx@...ts.freedesktop.org>,
        Tianlin Li <tli@...italocean.com>,
        Maling list - DRI developers 
        <dri-devel@...ts.freedesktop.org>,
        Alex Deucher <alexander.deucher@....com>
Subject: Re: [PATCH 0/2] drm/radeon: have the callers of set_memory_*() check
 the return value

Hi

Am 09.01.20 um 11:15 schrieb Christian König:
> Am 08.01.20 um 18:51 schrieb Alex Deucher:
>> On Wed, Jan 8, 2020 at 12:39 PM Kees Cook <keescook@...omium.org> wrote:
>>> On Wed, Jan 08, 2020 at 01:56:47PM +0100, Christian König wrote:
>>>> Am 07.01.20 um 20:25 schrieb Tianlin Li:
>>>>> Right now several architectures allow their set_memory_*() family of
>>>>> functions to fail, but callers may not be checking the return values.
>>>>> If set_memory_*() returns with an error, call-site assumptions may be
>>>>> infact wrong to assume that it would either succeed or not succeed at
>>>>> all. Ideally, the failure of set_memory_*() should be passed up the
>>>>> call stack, and callers should examine the failure and deal with it.
>>>>>
>>>>> Need to fix the callers and add the __must_check attribute. They also
>>>>> may not provide any level of atomicity, in the sense that the memory
>>>>> protections may be left incomplete on failure. This issue likely has a
>>>>> few steps on effects architectures:
>>>>> 1)Have all callers of set_memory_*() helpers check the return value.
>>>>> 2)Add __must_check to all set_memory_*() helpers so that new uses do
>>>>> not ignore the return value.
>>>>> 3)Add atomicity to the calls so that the memory protections aren't
>>>>> left
>>>>> in a partial state.
>>>>>
>>>>> This series is part of step 1. Make drm/radeon check the return
>>>>> value of
>>>>> set_memory_*().
>>>> I'm a little hesitate merge that. This hardware is >15 years old and
>>>> nobody
>>>> of the developers have any system left to test this change on.
>>> If that's true it should be removed from the tree. We need to be able to
>>> correctly make these kinds of changes in the kernel.
>> This driver supports about 15 years of hardware generations.  Newer
>> cards are still prevalent, but the older stuff is less so.  It still
>> works and people use it based on feedback I've seen, but the older
>> stuff has no active development at this point.  This change just
>> happens to target those older chips.
> 
> Just a few weeks back we've got a mail from somebody using an integrated
> R128 in a laptop.
> 
> After a few mails back and force we figured out that his nearly 20 years
> old hardware was finally failing.
> 
> Up till that he was still successfully updating his kernel from time to
> time and the driver still worked. I find that pretty impressive.
> 
>>
>> Alex
>>
>>>> Would it be to much of a problem to just add something like: r =
>>>> set_memory_*(); (void)r; /* Intentionally ignored */.
>>> This seems like a bad idea -- we shouldn't be papering over failures
>>> like this when there is logic available to deal with it.
> 
> Well I certainly agree to that, but we are talking about a call which
> happens only once during driver load/unload. If necessary we could also
> print an error when something goes wrong, but please no larger
> refactoring of return values and call paths.
> 

IMHO radeon should be marked as orphaned or obsolete then.

Best regards
Thomas

> It is perfectly possible that this call actually failed on somebodies
> hardware, but we never noticed because the driver still works fine. If
> we now handle the error it is possible that the module never loads and
> the user gets a black screen instead.
> 
> Regards,
> Christian.
> 
>>>
>>>> Apart from that certainly a good idea to add __must_check to the
>>>> functions.
>>> Agreed!
>>>
>>> -Kees
>>>
>>> -- 
>>> Kees Cook
>>> _______________________________________________
>>> dri-devel mailing list
>>> dri-devel@...ts.freedesktop.org
>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel&amp;data=02%7C01%7Cchristian.koenig%40amd.com%7Ca542d384d54040b5b0b708d794636df1%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637141027080080147&amp;sdata=EHFl6YOHmNp7gOqWsVmfoeD0jNirBTOGHcCP4efC%2FvE%3D&amp;reserved=0
>>>
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@...ts.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer



Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)

Powered by blists - more mailing lists