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 <[email protected]>
---
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
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?
On Thu, Oct 18, 2018 at 4:13 AM Mika Westerberg
<[email protected]> 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
On Sat, Oct 20, 2018 at 12:25 AM Wenwen Wang <[email protected]> wrote:
>
> On Thu, Oct 18, 2018 at 4:13 AM Mika Westerberg
> <[email protected]> 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.
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!