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 <[email protected]>
---
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
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 :)
On Mon, Oct 22, 2018 at 3:04 AM Mika Westerberg
<[email protected]> 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