In icm_copy(), the packet id 'hdr->packet_id' is firstly compared against
'req->npackets'. If it is less than 'req->npackets', the received packet.
i.e., 'pkg->buffer', is then copied to 'req->response + offset' through
memcpy(). It is worth noting that 'offset' is also calculated based on
'hdr->packet_id'. The problem here is that both the check and the
calculation are conducted directly on 'pkg->buffer', which is actually a
DMA memory region. Given that a device can also access the DMA region at
any time, it is possible that a malicious device controlled by an attacker
can modify the packet id after the check. By doing so, the attacker can
supply comprised value into 'offset' and thus cause unexpected errors.
This patch firstly copies the header of the packet and performs the check
and the calculation on the copied version to fix the above issue. This
patch also rewrites the header in 'req->response + offset' using the
copied header to avoid a potential inconsistency issue.
Signed-off-by: Wenwen Wang <[email protected]>
---
drivers/thunderbolt/icm.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/drivers/thunderbolt/icm.c b/drivers/thunderbolt/icm.c
index 28fc4ce..3beec4b 100644
--- a/drivers/thunderbolt/icm.c
+++ b/drivers/thunderbolt/icm.c
@@ -186,15 +186,18 @@ static bool icm_match(const struct tb_cfg_request *req,
static bool icm_copy(struct tb_cfg_request *req, const struct ctl_pkg *pkg)
{
- const struct icm_pkg_header *hdr = pkg->buffer;
+ struct icm_pkg_header hdr;
- if (hdr->packet_id < req->npackets) {
- size_t offset = hdr->packet_id * req->response_size;
+ memcpy(&hdr, pkg->buffer, sizeof(hdr));
+
+ if (hdr.packet_id < req->npackets) {
+ size_t offset = hdr.packet_id * req->response_size;
memcpy(req->response + offset, pkg->buffer, req->response_size);
+ (struct icm_pkg_header *)(req->response + offset) = hdr;
}
- return hdr->packet_id == hdr->total_packets - 1;
+ return hdr.packet_id == hdr.total_packets - 1;
}
static int icm_request(struct tb *tb, const void *request, size_t request_size,
--
2.7.4
Hi,
On Sat, Oct 20, 2018 at 01:38:18PM -0500, Wenwen Wang wrote:
> In icm_copy(), the packet id 'hdr->packet_id' is firstly compared against
> 'req->npackets'. If it is less than 'req->npackets', the received packet.
> i.e., 'pkg->buffer', is then copied to 'req->response + offset' through
> memcpy(). It is worth noting that 'offset' is also calculated based on
> 'hdr->packet_id'. The problem here is that both the check and the
> calculation are conducted directly on 'pkg->buffer', which is actually a
> DMA memory region. Given that a device can also access the DMA region at
> any time, it is possible that a malicious device controlled by an attacker
> can modify the packet id after the check. By doing so, the attacker can
> supply comprised value into 'offset' and thus cause unexpected errors.
>
> This patch firstly copies the header of the packet and performs the check
> and the calculation on the copied version to fix the above issue. This
> patch also rewrites the header in 'req->response + offset' using the
> copied header to avoid a potential inconsistency issue.
Same comment here than with the previous one.