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;
Date:   Mon, 22 Oct 2018 11:05:26 +0300
From:   Mika Westerberg <mika.westerberg@linux.intel.com>
To:     Wenwen Wang <wang6495@umn.edu>
Cc:     Kangjie Lu <kjlu@umn.edu>,
        Andreas Noever <andreas.noever@gmail.com>,
        Michael Jamet <michael.jamet@intel.com>,
        Yehezkel Bernat <YehezkelShB@gmail.com>,
        open list <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] thunderbolt: fix a missing-check bug
Message-ID: <20181022080526.GL2302@lahna.fi.intel.com>
References: <1540060698-17331-1-git-send-email-wang6495@umn.edu>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1540060698-17331-1-git-send-email-wang6495@umn.edu>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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.