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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAAa=b7d9tV17gOa-kGR6soKHqF4UvZdBrFmV2ga4e=ra6xDrNg@mail.gmail.com>
Date:   Mon, 22 Oct 2018 08:02:14 -0500
From:   Wenwen Wang <wang6495@....edu>
To:     mika.westerberg@...ux.intel.com
Cc:     Kangjie Lu <kjlu@....edu>,
        Andreas Noever <andreas.noever@...il.com>,
        michael.jamet@...el.com, Yehezkel Bernat <YehezkelShB@...il.com>,
        open list <linux-kernel@...r.kernel.org>,
        Wenwen Wang <wang6495@....edu>
Subject: Re: [PATCH] thunderbolt: Fix a missing-check bug

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