Kernel-hardening archive on lore.kernel.org
 help / color / Atom feed
From: Kees Cook <keescook@chromium.org>
To: "Christian König" <christian.koenig@amd.com>
Cc: Tianlin Li <tli@digitalocean.com>,
	kernel-hardening@lists.openwall.com,
	Alex Deucher <alexander.deucher@amd.com>,
	David1.Zhou@amd.com, David Airlie <airlied@linux.ie>,
	Daniel Vetter <daniel@ffwll.ch>,
	amd-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org,
	linux-kernel@vger.kernel.org,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Subject: Re: [PATCH 0/2] drm/radeon: have the callers of set_memory_*() check the return value
Date: Wed, 8 Jan 2020 09:39:27 -0800
Message-ID: <202001080936.A36005F1@keescook> (raw)
In-Reply-To: <b5984995-7276-97d3-a604-ddacfb89bd89@amd.com>

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.

> 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.

> Apart from that certainly a good idea to add __must_check to the functions.

Agreed!

-Kees

-- 
Kees Cook

  parent 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 [this message]
2020-01-08 17:51     ` Alex Deucher
2020-01-09 10:15       ` Christian König
2020-01-09 10:49         ` Thomas Zimmermann
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=202001080936.A36005F1@keescook \
    --to=keescook@chromium.org \
    --cc=David1.Zhou@amd.com \
    --cc=airlied@linux.ie \
    --cc=alexander.deucher@amd.com \
    --cc=amd-gfx@lists.freedesktop.org \
    --cc=christian.koenig@amd.com \
    --cc=daniel@ffwll.ch \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=gregkh@linuxfoundation.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