Kernel-hardening archive on lore.kernel.org
 help / color / Atom feed
From: Thomas Zimmermann <tzimmermann@suse.de>
To: "Christian König" <christian.koenig@amd.com>,
	"Alex Deucher" <alexdeucher@gmail.com>,
	"Kees Cook" <keescook@chromium.org>
Cc: kernel-hardening@lists.openwall.com,
	David Airlie <airlied@linux.ie>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	LKML <linux-kernel@vger.kernel.org>,
	amd-gfx list <amd-gfx@lists.freedesktop.org>,
	Tianlin Li <tli@digitalocean.com>,
	Maling list - DRI developers <dri-devel@lists.freedesktop.org>,
	Alex Deucher <alexander.deucher@amd.com>
Subject: Re: [PATCH 0/2] drm/radeon: have the callers of set_memory_*() check the return value
Date: Thu, 9 Jan 2020 11:49:17 +0100
Message-ID: <505a76a9-6110-3ddb-0f15-059b60922482@suse.de> (raw)
In-Reply-To: <a2919283-f5aa-43b2-9186-6c41315458c4@amd.com>

[-- Attachment #1.1: Type: text/plain, Size: 4376 bytes --]

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@chromium.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@lists.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@lists.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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

  reply index

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-01-07 19:25 Tianlin Li
2020-01-07 19:25 ` [PATCH 1/2] " Tianlin Li
2020-01-07 19:25 ` [PATCH 2/2] drm/radeon: change call sites to handle return value properly Tianlin Li
2020-01-08 12:56 ` [PATCH 0/2] drm/radeon: have the callers of set_memory_*() check the return value Christian König
2020-01-08 16:04   ` Tianlin Li
2020-01-08 17:39   ` Kees Cook
2020-01-08 17:51     ` Alex Deucher
2020-01-09 10:15       ` Christian König
2020-01-09 10:49         ` Thomas Zimmermann [this message]
2020-01-09 20:16           ` Alex Deucher

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=505a76a9-6110-3ddb-0f15-059b60922482@suse.de \
    --to=tzimmermann@suse.de \
    --cc=airlied@linux.ie \
    --cc=alexander.deucher@amd.com \
    --cc=alexdeucher@gmail.com \
    --cc=amd-gfx@lists.freedesktop.org \
    --cc=christian.koenig@amd.com \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=keescook@chromium.org \
    --cc=kernel-hardening@lists.openwall.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=tli@digitalocean.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Kernel-hardening archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/kernel-hardening/0 kernel-hardening/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 kernel-hardening kernel-hardening/ https://lore.kernel.org/kernel-hardening \
		kernel-hardening@lists.openwall.com
	public-inbox-index kernel-hardening

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/com.openwall.lists.kernel-hardening


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git