Re: [PATCH] thunderbolt: Fix a missing-check bug

From: Mika Westerberg <mika.westerberg@linux.intel.com>
To: Wenwen Wang <wang6495@umn.edu>
Cc: Kangjie Lu <kjlu@umn.edu>,
	Andreas Noever <andreas.noever@gmail.com>,
	Michael Jamet <michael.jamet@intel.com>,
	Yehezkel Bernat <YehezkelShB@gmail.com>,
	open list <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] thunderbolt: Fix a missing-check bug
Date: Mon, 22 Oct 2018 11:04:49 +0300	[thread overview]
Message-ID: <20181022080449.GK2302@lahna.fi.intel.com> (raw)
In-Reply-To: <1540058151-17116-1-git-send-email-wang6495@umn.edu>

Hi,

On Sat, Oct 20, 2018 at 12:55:51PM -0500, Wenwen Wang wrote:
> In tb_ctl_rx_callback(), the checksum of the received control packet is
> calculated on 'pkg->buffer' through tb_crc() and saved to 'crc32', Then,
> 'crc32' is compared with the received checksum to confirm the integrity of
> the received packet. If the checksum does not match, the packet will be
> dropped. In the following execution, 'pkg->buffer' will be copied through
> req->copy() and processed if there is an active request and the packet is
> what is expected.
> 
> The problem here is that the above checking process is performed directly
> on the buffer 'pkg->buffer', which is actually a DMA region. Given that the
> DMA region can also be accessed directly by a device at any time, it is
> possible that a malicious device controlled by an attacker can race to
> modify the content in 'pkg->buffer' after the checksum checking but before
> req->copy(). By doing so, the attacker can inject malicious data, which can
> cause undefined behavior of the kernel and introduce potential security
> risk.
> 
> This patch allocates a new buffer 'buf' to hold the data in 'pkg->buffer'.
> By performing the checking and copying on 'buf', rather than 'pkg->buffer',
> the above issue can be avoided.

Here same comment applies than to the previous one - this is something
that requires the attacker to have physical access to the system and
requires him to either replace the firmware or the hardware itself with
a malicious one and in that case protection like this here does not
actually help because they can just overwrite it directly.

BTW, just in case you send multiple patches to other subsystems as well
it is good to have $subject contain summary of the fix in a way that one
can distinguish between them. For example you sent 4 patches with all
having:

  thunderbolt: Fix a missing-check bug

in the $subject. So for example I originally thought that you sent the
same patch several times :)