Kernel-hardening archive on lore.kernel.org
 help / color / Atom feed
From: Tianlin Li <tli@digitalocean.com>
To: "Christian König" <christian.koenig@amd.com>
Cc: kernel-hardening@lists.openwall.com, keescook@chromium.org,
	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
Subject: Re: [PATCH 0/2] drm/radeon: have the callers of set_memory_*() check the return value
Date: Wed, 8 Jan 2020 10:04:54 -0600
Message-ID: <45CCBFCA-AACA-4C5E-B554-9E511872EAB9@digitalocean.com> (raw)
In-Reply-To: <b5984995-7276-97d3-a604-ddacfb89bd89@amd.com>

[-- Attachment #1: Type: text/plain, Size: 2206 bytes --]


> On Jan 8, 2020, at 6:56 AM, Christian König <christian.koenig@amd.com> 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.
> 
> Would it be to much of a problem to just add something like: r = set_memory_*(); (void)r; /* Intentionally ignored */.

Thank you. I will fix that in patch 1 and remove patch 2 (since no need to fix the call sites to handle the retval). 

Best regards,
Tianlin
> Apart from that certainly a good idea to add __must_check to the functions.
> 
> Regards,
> Christian.
> 
>> 
>> Tianlin Li (2):
>>   drm/radeon: have the callers of set_memory_*() check the return value
>>   drm/radeon: change call sites to handle return value properly.
>> 
>>  drivers/gpu/drm/radeon/r100.c        |  3 ++-
>>  drivers/gpu/drm/radeon/radeon.h      |  2 +-
>>  drivers/gpu/drm/radeon/radeon_gart.c | 22 ++++++++++++++++++----
>>  drivers/gpu/drm/radeon/rs400.c       |  3 ++-
>>  4 files changed, 23 insertions(+), 7 deletions(-)


[-- Attachment #2: Type: text/html, Size: 9557 bytes --]

<html><head><meta http-equiv="Content-Type" content="text/html; charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class=""><br class=""><div><blockquote type="cite" class=""><div class="">On Jan 8, 2020, at 6:56 AM, Christian König &lt;<a href="mailto:christian.koenig@amd.com" class="">christian.koenig@amd.com</a>&gt; wrote:</div><br class="Apple-interchange-newline"><div class=""><span style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none; float: none; display: inline !important;" class="">Am 07.01.20 um 20:25 schrieb Tianlin Li:</span><br style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><blockquote type="cite" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; text-decoration: none;" class="">Right now several architectures allow their set_memory_*() family of<br class="">functions to fail, but callers may not be checking the return values.<br class="">If set_memory_*() returns with an error, call-site assumptions may be<br class="">infact wrong to assume that it would either succeed or not succeed at<br class="">all. Ideally, the failure of set_memory_*() should be passed up the<br class="">call stack, and callers should examine the failure and deal with it.<br class=""><br class="">Need to fix the callers and add the __must_check attribute. They also<br class="">may not provide any level of atomicity, in the sense that the memory<br class="">protections may be left incomplete on failure. This issue likely has a<br class="">few steps on effects architectures:<br class="">1)Have all callers of set_memory_*() helpers check the return value.<br class="">2)Add __must_check to all set_memory_*() helpers so that new uses do<br class="">not ignore the return value.<br class="">3)Add atomicity to the calls so that the memory protections aren't left<br class="">in a partial state.<br class=""><br class="">This series is part of step 1. Make drm/radeon check the return value of<br class="">set_memory_*().<br class=""></blockquote><br style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><span style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none; float: none; display: inline !important;" class="">I'm a little hesitate merge that. This hardware is &gt;15 years old and nobody of the developers have any system left to test this change on.</span><br style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><br style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><span style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none; float: none; display: inline !important;" class="">Would it be to much of a problem to just add something like: r = set_memory_*(); (void)r; /* Intentionally ignored */.</span><br style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""></div></blockquote><div><br class=""></div><div>Thank you. I will fix that in patch 1 and remove patch 2 (since no need to fix the call sites to handle the retval).&nbsp;</div><div><br class=""></div><div>Best regards,</div><div>Tianlin</div><blockquote type="cite" class=""><div class=""><span style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none; float: none; display: inline !important;" class="">Apart from that certainly a good idea to add __must_check to the functions.</span><br style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><br style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><span style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none; float: none; display: inline !important;" class="">Regards,</span><br style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><span style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none; float: none; display: inline !important;" class="">Christian.</span><br style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><br style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><blockquote type="cite" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><br class="">Tianlin Li (2):<br class="">&nbsp;&nbsp;drm/radeon: have the callers of set_memory_*() check the return value<br class="">&nbsp;&nbsp;drm/radeon: change call sites to handle return value properly.<br class=""><br class="">&nbsp;drivers/gpu/drm/radeon/r100.c &nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;| &nbsp;3 ++-<br class="">&nbsp;drivers/gpu/drm/radeon/radeon.h &nbsp;&nbsp;&nbsp;&nbsp;&nbsp;| &nbsp;2 +-<br class="">&nbsp;drivers/gpu/drm/radeon/radeon_gart.c | 22 ++++++++++++++++++----<br class="">&nbsp;drivers/gpu/drm/radeon/rs400.c &nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;| &nbsp;3 ++-<br class="">&nbsp;4 files changed, 23 insertions(+), 7 deletions(-)</blockquote></div></blockquote></div><br class=""></body></html>

  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 [this message]
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
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=45CCBFCA-AACA-4C5E-B554-9E511872EAB9@digitalocean.com \
    --to=tli@digitalocean.com \
    --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=keescook@chromium.org \
    --cc=kernel-hardening@lists.openwall.com \
    --cc=linux-kernel@vger.kernel.org \
    /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