[PATCH] thunderbolt: Fix a missing-check bug

[PATCH] thunderbolt: Fix a missing-check bug

[Wenwen Wang]

In tb_cfg_copy(), the header of the received control package, which is in
the buffer 'pkg->buffer', is firstly parsed through parse_header() to make
sure the header is in the expected format. In parse_header(), the header is
actually checked by invoking check_header(), which checks the 'unknown'
field of the header and the response route acquired through
'tb_cfg_get_route(header)'. If there is no error in this checking process,
the package, including the header, is then copied to 'req->response' in
tb_cfg_copy() through memcpy() and the parsing result is saved to
'req->result'.

The problem here is that the whole parsing and checking process is
conducted directly on the buffer 'pkg->buffer', which is actually a DMA
region and allocated through dma_pool_alloc() in tb_ctl_pkg_alloc(). Given
that the DMA region can also be accessed directly by a device at any time,
it is possible that a malicious device can race to modify the package data
after the parsing and checking process but before memcpy() is invoked in
tb_cfg_copy(). Through this way, the attacker can bypass the parsing and
checking process and inject malicious data. This can potentially cause
undefined behavior of the kernel and introduce unexpected security risk.

This patch firstly copies the header of the package to a stack variable
'hdr' and performs the parsing and checking process on 'hdr'. If there is
no error in this process, 'hdr' is then used to rewrite the header in
'req->response' after memcpy(). This way, the above issue can be avoided.

Signed-off-by: Wenwen Wang <wang6495@umn.edu>
---
 drivers/thunderbolt/ctl.c | 39 ++++++++++++++++++++++-----------------
 1 file changed, 22 insertions(+), 17 deletions(-)

diff --git a/drivers/thunderbolt/ctl.c b/drivers/thunderbolt/ctl.c
index 37a7f4c..ae4cd61 100644
--- a/drivers/thunderbolt/ctl.c
+++ b/drivers/thunderbolt/ctl.c
@@ -166,10 +166,9 @@ tb_cfg_request_find(struct tb_ctl *ctl, struct ctl_pkg *pkg)
 
 
 static int check_header(const struct ctl_pkg *pkg, u32 len,
-			enum tb_cfg_pkg_type type, u64 route)
+			enum tb_cfg_pkg_type type, u64 route,
+			const struct tb_cfg_header *hdr)
 {
-	struct tb_cfg_header *header = pkg->buffer;
-
 	/* check frame, TODO: frame flags */
 	if (WARN(len != pkg->frame.size,
 			"wrong framesize (expected %#x, got %#x)\n",
@@ -183,12 +182,12 @@ static int check_header(const struct ctl_pkg *pkg, u32 len,
 		return -EIO;
 
 	/* check header */
-	if (WARN(header->unknown != 1 << 9,
-			"header->unknown is %#x\n", header->unknown))
+	if (WARN(hdr->unknown != 1 << 9,
+			"hdr->unknown is %#x\n", hdr->unknown))
 		return -EIO;
-	if (WARN(route != tb_cfg_get_route(header),
+	if (WARN(route != tb_cfg_get_route(hdr),
 			"wrong route (expected %llx, got %llx)",
-			route, tb_cfg_get_route(header)))
+			route, tb_cfg_get_route(hdr)))
 		return -EIO;
 	return 0;
 }
@@ -215,14 +214,15 @@ static int check_config_address(struct tb_cfg_address addr,
 	return 0;
 }
 
-static struct tb_cfg_result decode_error(const struct ctl_pkg *response)
+static struct tb_cfg_result decode_error(const struct ctl_pkg *response,
+					 const struct tb_cfg_header *hdr)
 {
 	struct cfg_error_pkg *pkg = response->buffer;
 	struct tb_cfg_result res = { 0 };
-	res.response_route = tb_cfg_get_route(&pkg->header);
+	res.response_route = tb_cfg_get_route(hdr);
 	res.response_port = 0;
 	res.err = check_header(response, sizeof(*pkg), TB_CFG_PKG_ERROR,
-			       tb_cfg_get_route(&pkg->header));
+			       tb_cfg_get_route(hdr), hdr);
 	if (res.err)
 		return res;
 
@@ -237,17 +237,17 @@ static struct tb_cfg_result decode_error(const struct ctl_pkg *response)
 }
 
 static struct tb_cfg_result parse_header(const struct ctl_pkg *pkg, u32 len,
-					 enum tb_cfg_pkg_type type, u64 route)
+					 enum tb_cfg_pkg_type type, u64 route,
+					 const struct tb_cfg_header *hdr)
 {
-	struct tb_cfg_header *header = pkg->buffer;
 	struct tb_cfg_result res = { 0 };
 
 	if (pkg->frame.eof == TB_CFG_PKG_ERROR)
-		return decode_error(pkg);
+		return decode_error(pkg, hdr);
 
 	res.response_port = 0; /* will be updated later for cfg_read/write */
-	res.response_route = tb_cfg_get_route(header);
-	res.err = check_header(pkg, len, type, route);
+	res.response_route = tb_cfg_get_route(hdr);
+	res.err = check_header(pkg, len, type, route, hdr);
 	return res;
 }
 
@@ -753,13 +753,18 @@ static bool tb_cfg_match(const struct tb_cfg_request *req,
 
 static bool tb_cfg_copy(struct tb_cfg_request *req, const struct ctl_pkg *pkg)
 {
+	struct tb_cfg_header hdr;
 	struct tb_cfg_result res;
 
+	hdr = *(struct tb_cfg_header *)pkg->buffer;
+
 	/* Now make sure it is in expected format */
 	res = parse_header(pkg, req->response_size, req->response_type,
-			   tb_cfg_get_route(req->request));
-	if (!res.err)
+			   tb_cfg_get_route(req->request), &hdr);
+	if (!res.err) {
 		memcpy(req->response, pkg->buffer, req->response_size);
+		*(struct tb_cfg_header *)req->response = hdr;
+	}
 
 	req->result = res;
 
-- 
2.7.4

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

[Mika Westerberg]

Hi Wenwen,

On Wed, Oct 17, 2018 at 09:00:29AM -0500, Wenwen Wang wrote:
> In tb_cfg_copy(), the header of the received control package, which is in
> the buffer 'pkg->buffer', is firstly parsed through parse_header() to make
> sure the header is in the expected format. In parse_header(), the header is
> actually checked by invoking check_header(), which checks the 'unknown'
> field of the header and the response route acquired through
> 'tb_cfg_get_route(header)'. If there is no error in this checking process,
> the package, including the header, is then copied to 'req->response' in
> tb_cfg_copy() through memcpy() and the parsing result is saved to
> 'req->result'.
> 
> The problem here is that the whole parsing and checking process is
> conducted directly on the buffer 'pkg->buffer', which is actually a DMA
> region and allocated through dma_pool_alloc() in tb_ctl_pkg_alloc(). Given
> that the DMA region can also be accessed directly by a device at any time,
> it is possible that a malicious device can race to modify the package data
> after the parsing and checking process but before memcpy() is invoked in
> tb_cfg_copy(). Through this way, the attacker can bypass the parsing and
> checking process and inject malicious data. This can potentially cause
> undefined behavior of the kernel and introduce unexpected security risk.

Here the device doing DMA is the Thunderbolt host controller which is
soldered on the motherboard (not anything connected via the TBT ports).
In addition the buffers we are dealing here are already marked ready by
the host controller hardware so it is not expected to touch them anymore
(if it did, then it would be a quite nasty bug).

What kind of use-case you had in mind that could possibly inject
malicious data to these buffers?

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

[Wenwen Wang]

On Thu, Oct 18, 2018 at 4:13 AM Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
>
> Hi Wenwen,
>
> On Wed, Oct 17, 2018 at 09:00:29AM -0500, Wenwen Wang wrote:
> > In tb_cfg_copy(), the header of the received control package, which is in
> > the buffer 'pkg->buffer', is firstly parsed through parse_header() to make
> > sure the header is in the expected format. In parse_header(), the header is
> > actually checked by invoking check_header(), which checks the 'unknown'
> > field of the header and the response route acquired through
> > 'tb_cfg_get_route(header)'. If there is no error in this checking process,
> > the package, including the header, is then copied to 'req->response' in
> > tb_cfg_copy() through memcpy() and the parsing result is saved to
> > 'req->result'.
> >
> > The problem here is that the whole parsing and checking process is
> > conducted directly on the buffer 'pkg->buffer', which is actually a DMA
> > region and allocated through dma_pool_alloc() in tb_ctl_pkg_alloc(). Given
> > that the DMA region can also be accessed directly by a device at any time,
> > it is possible that a malicious device can race to modify the package data
> > after the parsing and checking process but before memcpy() is invoked in
> > tb_cfg_copy(). Through this way, the attacker can bypass the parsing and
> > checking process and inject malicious data. This can potentially cause
> > undefined behavior of the kernel and introduce unexpected security risk.
>
> Here the device doing DMA is the Thunderbolt host controller which is
> soldered on the motherboard (not anything connected via the TBT ports).
> In addition the buffers we are dealing here are already marked ready by
> the host controller hardware so it is not expected to touch them anymore
> (if it did, then it would be a quite nasty bug).
>
> What kind of use-case you had in mind that could possibly inject
> malicious data to these buffers?

Hi Mika,

Thanks for your response. The current version of the code assumes that
the Thunderbolt controller behaves as expected, e.g., the host
controller should not touch the data after it is marked ready.
However, it is not impossible that the controller is exploited by an
attacker through a security vulnerability, even though it is soldered
on the motherboard. In that case, the controller may behave in an
unexpected way and this bug will offer more opportunities for the
attacker.

Wenwen

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

[Yehezkel Bernat]

On Sat, Oct 20, 2018 at 12:25 AM Wenwen Wang <wang6495@umn.edu> wrote:
>
> On Thu, Oct 18, 2018 at 4:13 AM Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> >
> > Hi Wenwen,
> >
> > On Wed, Oct 17, 2018 at 09:00:29AM -0500, Wenwen Wang wrote:
> > > In tb_cfg_copy(), the header of the received control package, which is in
> > > the buffer 'pkg->buffer', is firstly parsed through parse_header() to make
> > > sure the header is in the expected format. In parse_header(), the header is
> > > actually checked by invoking check_header(), which checks the 'unknown'
> > > field of the header and the response route acquired through
> > > 'tb_cfg_get_route(header)'. If there is no error in this checking process,
> > > the package, including the header, is then copied to 'req->response' in
> > > tb_cfg_copy() through memcpy() and the parsing result is saved to
> > > 'req->result'.
> > >
> > > The problem here is that the whole parsing and checking process is
> > > conducted directly on the buffer 'pkg->buffer', which is actually a DMA
> > > region and allocated through dma_pool_alloc() in tb_ctl_pkg_alloc(). Given
> > > that the DMA region can also be accessed directly by a device at any time,
> > > it is possible that a malicious device can race to modify the package data
> > > after the parsing and checking process but before memcpy() is invoked in
> > > tb_cfg_copy(). Through this way, the attacker can bypass the parsing and
> > > checking process and inject malicious data. This can potentially cause
> > > undefined behavior of the kernel and introduce unexpected security risk.
> >
> > Here the device doing DMA is the Thunderbolt host controller which is
> > soldered on the motherboard (not anything connected via the TBT ports).
> > In addition the buffers we are dealing here are already marked ready by
> > the host controller hardware so it is not expected to touch them anymore
> > (if it did, then it would be a quite nasty bug).
> >
> > What kind of use-case you had in mind that could possibly inject
> > malicious data to these buffers?
>
> Hi Mika,
>
> Thanks for your response. The current version of the code assumes that
> the Thunderbolt controller behaves as expected, e.g., the host
> controller should not touch the data after it is marked ready.
> However, it is not impossible that the controller is exploited by an
> attacker through a security vulnerability, even though it is soldered
> on the motherboard. In that case, the controller may behave in an
> unexpected way and this bug will offer more opportunities for the
> attacker.


[Re-sending as plain text. Mobile clients aren't good at that,
apparently... Sorry.]

If a device that can do DMA (i.e. accessing the whole physical memory
directly) is compromised, incorrect read length is the least of the
troubles the user should worry about.

On the other hand, this is a performance-sensitive path of the code
and copying each DMA buffer will hurt the performance very much for
sure.

I agree with Mika that it doesn't make much sense to degrade the
performance here just for covering this theoretical attack that can do
much worse things anyway.

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

[Mika Westerberg]

On Fri, Oct 19, 2018 at 04:25:01PM -0500, Wenwen Wang wrote:
> Hi Mika,

Hi,

> Thanks for your response. The current version of the code assumes that
> the Thunderbolt controller behaves as expected, e.g., the host
> controller should not touch the data after it is marked ready.
> However, it is not impossible that the controller is exploited by an
> attacker through a security vulnerability, even though it is soldered
> on the motherboard. In that case, the controller may behave in an
> unexpected way and this bug will offer more opportunities for the
> attacker.

That would require the attacker to dissassemble the laptop case or
similar in case of desktop system. That's already something we cannot
protect against.

Furthermore this would apply to all DMA capable devices such as the xHCI
controller typically part of the Thunderbolt host router or every single
network card but I have not seen fixes like this on network side
(probably because there is really no need).

If the attacker could somehow say, replace the firmware on the
Thunderbolt host router then I suppose they could just go and overwrite
the extra protection you did in this patch (or probably do something
worse since they can access all the system memory).

So all in all I don't think this is something we would need to deal
with.

Situation is totally different if you manage to connecte external
devices that can do DMA (which is pretty much what Thunderbolt for
example allows) but for those we already have security of some sort
implemented.

Thanks!

[PATCH] thunderbolt: Fix a missing-check bug

[Wenwen Wang]

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.

Signed-off-by: Wenwen Wang <wang6495@umn.edu>
---
 drivers/thunderbolt/ctl.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/thunderbolt/ctl.c b/drivers/thunderbolt/ctl.c
index 37a7f4c..9e40572 100644
--- a/drivers/thunderbolt/ctl.c
+++ b/drivers/thunderbolt/ctl.c
@@ -409,6 +409,8 @@ static void tb_ctl_rx_callback(struct tb_ring *ring, struct ring_frame *frame,
 	struct ctl_pkg *pkg = container_of(frame, typeof(*pkg), frame);
 	struct tb_cfg_request *req;
 	__be32 crc32;
+	void *pkg_buf = pkg->buffer;
+	void *buf = NULL;
 
 	if (canceled)
 		return; /*
@@ -422,6 +424,13 @@ static void tb_ctl_rx_callback(struct tb_ring *ring, struct ring_frame *frame,
 		goto rx;
 	}
 
+	buf = kzalloc(frame->size, GFP_KERNEL);
+	if (!buf)
+		goto rx;
+
+	memcpy(buf, pkg->buffer, frame->size);
+	pkg->buffer = buf;
+
 	frame->size -= 4; /* remove checksum */
 	crc32 = tb_crc(pkg->buffer, frame->size);
 	be32_to_cpu_array(pkg->buffer, pkg->buffer, frame->size / 4);
@@ -476,6 +485,10 @@ static void tb_ctl_rx_callback(struct tb_ring *ring, struct ring_frame *frame,
 	}
 
 rx:
+	if (buf) {
+		pkg->buffer = pkg_buf;
+		kfree(buf);
+	}
 	tb_ctl_rx_submit(pkg);
 }
 
-- 
2.7.4

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

[Mika Westerberg]

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 :)

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

[Wenwen Wang]

On Mon, Oct 22, 2018 at 3:04 AM Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
>
> 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 :)

Thanks for your suggestion, Mika. That is good to distinguish between
different patches :)

Wenwen

[PATCH] thunderbolt: fix a missing-check bug

[Wenwen Wang]

In icm_copy(), the packet id 'hdr->packet_id' is firstly compared against
'req->npackets'. If it is less than 'req->npackets', the received packet.
i.e., 'pkg->buffer', is then copied to 'req->response + offset' through
memcpy(). It is worth noting that 'offset' is also calculated based on
'hdr->packet_id'. The problem here is that both the check and the
calculation are conducted directly on 'pkg->buffer', which is actually a
DMA memory region. Given that a device can also access the DMA region at
any time, it is possible that a malicious device controlled by an attacker
can modify the packet id after the check. By doing so, the attacker can
supply comprised value into 'offset' and thus cause unexpected errors.

This patch firstly copies the header of the packet and performs the check
and the calculation on the copied version to fix the above issue. This
patch also rewrites the header in 'req->response + offset' using the
copied header to avoid a potential inconsistency issue.

Signed-off-by: Wenwen Wang <wang6495@umn.edu>
---
 drivers/thunderbolt/icm.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/thunderbolt/icm.c b/drivers/thunderbolt/icm.c
index 28fc4ce..3beec4b 100644
--- a/drivers/thunderbolt/icm.c
+++ b/drivers/thunderbolt/icm.c
@@ -186,15 +186,18 @@ static bool icm_match(const struct tb_cfg_request *req,
 
 static bool icm_copy(struct tb_cfg_request *req, const struct ctl_pkg *pkg)
 {
-	const struct icm_pkg_header *hdr = pkg->buffer;
+	struct icm_pkg_header hdr;
 
-	if (hdr->packet_id < req->npackets) {
-		size_t offset = hdr->packet_id * req->response_size;
+	memcpy(&hdr, pkg->buffer, sizeof(hdr));
+
+	if (hdr.packet_id < req->npackets) {
+		size_t offset = hdr.packet_id * req->response_size;
 
 		memcpy(req->response + offset, pkg->buffer, req->response_size);
+		(struct icm_pkg_header *)(req->response + offset) = hdr;
 	}
 
-	return hdr->packet_id == hdr->total_packets - 1;
+	return hdr.packet_id == hdr.total_packets - 1;
 }
 
 static int icm_request(struct tb *tb, const void *request, size_t request_size,
-- 
2.7.4

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

[Mika Westerberg]

Hi,

On Sat, Oct 20, 2018 at 01:38:18PM -0500, Wenwen Wang wrote:
> In icm_copy(), the packet id 'hdr->packet_id' is firstly compared against
> 'req->npackets'. If it is less than 'req->npackets', the received packet.
> i.e., 'pkg->buffer', is then copied to 'req->response + offset' through
> memcpy(). It is worth noting that 'offset' is also calculated based on
> 'hdr->packet_id'. The problem here is that both the check and the
> calculation are conducted directly on 'pkg->buffer', which is actually a
> DMA memory region. Given that a device can also access the DMA region at
> any time, it is possible that a malicious device controlled by an attacker
> can modify the packet id after the check. By doing so, the attacker can
> supply comprised value into 'offset' and thus cause unexpected errors.
> 
> This patch firstly copies the header of the packet and performs the check
> and the calculation on the copied version to fix the above issue. This
> patch also rewrites the header in 'req->response + offset' using the
> copied header to avoid a potential inconsistency issue.

Same comment here than with the previous one.

[PATCH] thunderbolt: fix a missing-check bug

[Wenwen Wang]

In ring_work(), the first while loop is used to collect all completed
frames from the ring buffer. In each iteration of this loop, the flag of
the frame, i.e., 'ring->descriptors[ring->tail].flags' is firstly check to
see whether the frame is completed. If yes, the descriptor of the frame,
including the flag, is then copied. It is worth noting that the descriptor
is actually in a DMA region, which is allocated through
dma_alloc_coherent() in tb_ring_alloc(). Given that the device can also
access the DMA region, a malicious device controlled by an attacker can
race to modify the flag of the frame after the check but before the copy.
By doing so, the attacker can bypass the check and supply uncompleted
frame, which can cause undefined behavior of the kernel and introduce
potential security risk.

This patch firstly copies the flag into a local variable 'desc_flags' and
then performs the check and copy using 'desc_flags'. Through this way, the
above issue can be avoided.

Signed-off-by: Wenwen Wang <wang6495@umn.edu>
---
 drivers/thunderbolt/nhi.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/thunderbolt/nhi.c b/drivers/thunderbolt/nhi.c
index 5cd6bdf..22bd6cf 100644
--- a/drivers/thunderbolt/nhi.c
+++ b/drivers/thunderbolt/nhi.c
@@ -215,6 +215,7 @@ static void ring_work(struct work_struct *work)
 	struct ring_frame *frame;
 	bool canceled = false;
 	unsigned long flags;
+	enum ring_desc_flags desc_flags;
 	LIST_HEAD(done);
 
 	spin_lock_irqsave(&ring->lock, flags);
@@ -228,8 +229,8 @@ static void ring_work(struct work_struct *work)
 	}
 
 	while (!ring_empty(ring)) {
-		if (!(ring->descriptors[ring->tail].flags
-				& RING_DESC_COMPLETED))
+		desc_flags = ring->descriptors[ring->tail].flags;
+		if (!(desc_flags & RING_DESC_COMPLETED))
 			break;
 		frame = list_first_entry(&ring->in_flight, typeof(*frame),
 					 list);
@@ -238,7 +239,7 @@ static void ring_work(struct work_struct *work)
 			frame->size = ring->descriptors[ring->tail].length;
 			frame->eof = ring->descriptors[ring->tail].eof;
 			frame->sof = ring->descriptors[ring->tail].sof;
-			frame->flags = ring->descriptors[ring->tail].flags;
+			frame->flags = desc_flags;
 		}
 		ring->tail = (ring->tail + 1) % ring->size;
 	}
-- 
2.7.4

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

[Mika Westerberg]

On Sat, Oct 20, 2018 at 02:47:49PM -0500, Wenwen Wang wrote:
> In ring_work(), the first while loop is used to collect all completed
> frames from the ring buffer. In each iteration of this loop, the flag of
> the frame, i.e., 'ring->descriptors[ring->tail].flags' is firstly check to
> see whether the frame is completed. If yes, the descriptor of the frame,
> including the flag, is then copied. It is worth noting that the descriptor
> is actually in a DMA region, which is allocated through
> dma_alloc_coherent() in tb_ring_alloc(). Given that the device can also
> access the DMA region, a malicious device controlled by an attacker can
> race to modify the flag of the frame after the check but before the copy.
> By doing so, the attacker can bypass the check and supply uncompleted
> frame, which can cause undefined behavior of the kernel and introduce
> potential security risk.
> 
> This patch firstly copies the flag into a local variable 'desc_flags' and
> then performs the check and copy using 'desc_flags'. Through this way, the
> above issue can be avoided.

Ditto :)

[PATCH] thunderbolt: fix a missing-check bug

[Wenwen Wang]

In tb_ring_poll(), the flag of the frame, i.e.,
'ring->descriptors[ring->tail].flags', is checked to see whether the frame
is completed. If yes, the frame including the flag will be read from the
ring and returned to the caller. The problem here is that the flag is
actually in a DMA region, which is allocated through dma_alloc_coherent()
in tb_ring_alloc(). Given that the device can also access this DMA region,
it is possible that a malicious device controlled by an attacker can modify
the flag between the check and the copy. By doing so, the attacker can
bypass the check and supply uncompleted frame, which can cause undefined
behavior of the kernel and introduce potential security risk.

This patch firstly copies the flag into a local variable 'desc_flags' and
then performs the check and copy using 'desc_flags'. Through this way, the
above issue can be avoided.

Signed-off-by: Wenwen Wang <wang6495@umn.edu>
---
 drivers/thunderbolt/nhi.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/thunderbolt/nhi.c b/drivers/thunderbolt/nhi.c
index 5cd6bdf..481b1f2 100644
--- a/drivers/thunderbolt/nhi.c
+++ b/drivers/thunderbolt/nhi.c
@@ -289,6 +289,7 @@ struct ring_frame *tb_ring_poll(struct tb_ring *ring)
 {
 	struct ring_frame *frame = NULL;
 	unsigned long flags;
+	enum ring_desc_flags desc_flags;
 
 	spin_lock_irqsave(&ring->lock, flags);
 	if (!ring->running)
@@ -296,7 +297,8 @@ struct ring_frame *tb_ring_poll(struct tb_ring *ring)
 	if (ring_empty(ring))
 		goto unlock;
 
-	if (ring->descriptors[ring->tail].flags & RING_DESC_COMPLETED) {
+	desc_flags = ring->descriptors[ring->tail].flags;
+	if (desc_flags & RING_DESC_COMPLETED) {
 		frame = list_first_entry(&ring->in_flight, typeof(*frame),
 					 list);
 		list_del_init(&frame->list);
@@ -305,7 +307,7 @@ struct ring_frame *tb_ring_poll(struct tb_ring *ring)
 			frame->size = ring->descriptors[ring->tail].length;
 			frame->eof = ring->descriptors[ring->tail].eof;
 			frame->sof = ring->descriptors[ring->tail].sof;
-			frame->flags = ring->descriptors[ring->tail].flags;
+			frame->flags = desc_flags;
 		}
 
 		ring->tail = (ring->tail + 1) % ring->size;
-- 
2.7.4

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

[Mika Westerberg]

On Sat, Oct 20, 2018 at 03:15:56PM -0500, Wenwen Wang wrote:
> In tb_ring_poll(), the flag of the frame, i.e.,
> 'ring->descriptors[ring->tail].flags', is checked to see whether the frame
> is completed. If yes, the frame including the flag will be read from the
> ring and returned to the caller. The problem here is that the flag is
> actually in a DMA region, which is allocated through dma_alloc_coherent()
> in tb_ring_alloc(). Given that the device can also access this DMA region,
> it is possible that a malicious device controlled by an attacker can modify
> the flag between the check and the copy. By doing so, the attacker can
> bypass the check and supply uncompleted frame, which can cause undefined
> behavior of the kernel and introduce potential security risk.
> 
> This patch firstly copies the flag into a local variable 'desc_flags' and
> then performs the check and copy using 'desc_flags'. Through this way, the
> above issue can be avoided.

Same here :)