Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <1539784829-1159-1-git-send-email-wang6495@umn.edu>
 <20181018091319.GT2302@lahna.fi.intel.com> <CAAa=b7f-Q2ySZt2vqa1L2GyTA1FrT7+7_ui-eSLEYgN6iXSTFQ@mail.gmail.com>
In-Reply-To: <CAAa=b7f-Q2ySZt2vqa1L2GyTA1FrT7+7_ui-eSLEYgN6iXSTFQ@mail.gmail.com>
From:   Yehezkel Bernat <yehezkelshb@gmail.com>
Date:   Sat, 20 Oct 2018 21:47:29 +0300
Message-ID: <CA+CmpXsy1E+WY6cxMmzLZKhVZ5G78NfqPAKirT9qxJJpkYn1=Q@mail.gmail.com>
Subject: Re: [PATCH] thunderbolt: Fix a missing-check bug
To:     wang6495@umn.edu
Cc:     Mika Westerberg <mika.westerberg@linux.intel.com>, kjlu@umn.edu,
        Andreas Noever <andreas.noever@gmail.com>,
        michael.jamet@intel.com, LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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.