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

[Wenwen Wang]

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