Received: by 2002:ac0:a5a6:0:0:0:0:0 with SMTP id m35-v6csp2887396imm; Tue, 4 Sep 2018 11:35:52 -0700 (PDT) X-Google-Smtp-Source: ANB0VdYHYcke91dB7p/eCeu2w4mbBPtJuCbctaCDlaKHJRrZI11/U3M86l9QOkKvPfxGt/6rJk/7 X-Received: by 2002:a63:f657:: with SMTP id u23-v6mr32638704pgj.258.1536086152839; Tue, 04 Sep 2018 11:35:52 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1536086152; cv=none; d=google.com; s=arc-20160816; b=nSxYEVkXzfycS+3yhzSw1uWB+Wb0nHBt9CY4/FpBmD0PWLFtPDWU1chELSTDF+khep 9m4YWhnAkKzur4PDq4fttcUXweNsctw9UkaNUQztO/pPc7cAaj2l3Au92m6MY3KWxbyU mUrJe7+7S7PVQ2jkrBtaLq4Y9TrusTjtolLRHZBdCqixWm6XII4+JT711bTVb8dlzgRn EEKlTLYednvF2zo+g/qDcmNTdjO3ZBHpgLi6mwKE7kZlyFJKXaiQxASDGt6+Cf9aB6A2 0oi6QOHUEd6a4vzJ9IL+PeL+j5eRUuzzpx2ZiWgdTiTTF5CSBJJA8KNgMbBuSo7oM95u 8YGg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:content-transfer-encoding :content-language:in-reply-to:mime-version:user-agent:date :message-id:from:references:cc:to:subject:arc-authentication-results; bh=Onjyzaf7MAvXJkk7WckEfZHjdVETkK9VpjfNJFgsVlw=; b=PWeOTHXcp5HssVmh8zWr3YzYMMP4WSljvvfggGRPLKp1u6SFB7Q0GHe7nCpRRHLnJh cQ0ykMgSk42/zAqQtvGZUduLNYuvLWjHE6HVVZK9MMIkOAibO9lSB2SnFN3LIHHHvDQ+ UtKaNh/EIKKRPFeVILo0OxYy3tbb5VsaAPHQcu/tz3jat1ANxbwIWA4UCoYpv8vUxR5S Iwar6zv37ONE9isfwJuu6tbv4NxLIVt6rRcUh5jkydONK5Q4iRhZl+WWI3qk9OBzri6k jbW30DcxpmvG708WW1ekwKHR1zLHEozzqulNugH5+b5ZISPcp99oOt7zzB0uViqII3At jOWA== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=redhat.com Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id b19-v6si24289339pfb.89.2018.09.04.11.35.37; Tue, 04 Sep 2018 11:35:52 -0700 (PDT) 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; Authentication-Results: mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=redhat.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727990AbeIDXAK (ORCPT + 99 others); Tue, 4 Sep 2018 19:00:10 -0400 Received: from mail-qk1-f193.google.com ([209.85.222.193]:36906 "EHLO mail-qk1-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726507AbeIDXAJ (ORCPT ); Tue, 4 Sep 2018 19:00:09 -0400 Received: by mail-qk1-f193.google.com with SMTP id f17-v6so3121353qkh.4 for ; Tue, 04 Sep 2018 11:33:49 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=Onjyzaf7MAvXJkk7WckEfZHjdVETkK9VpjfNJFgsVlw=; b=bPVORJEQWK869Ws9C4yNFPd5VnBF8gNSOrkMO1W/vPqh4lXFubaMvQUWeQ7ohwS6Hc 6Ns1ZcK7BtZYLz9RF6HQ/xZxuB8ZHj3MhD3IZWn96LvsKTi2m8XO2QUgjeRRLtDtZcia CGsZsFa8uh4uPjgXe0yuaaubbj4sxG6oYSUc30IqU9/Br9xmRWdaCzEpZ2/Cs/Mk5R2V mZXopB98GANmEXkNblQyHGDHBwrq1lC+oKoX/4BFFazxJxyQm2bOAFRk4GYOydRySFSh nHcj3e5Z/Jim1jFi/90PLeJRMV1dB4QcoogFdEILZnO97LUb2WHEH3scvzjRbT/HObha dy9w== X-Gm-Message-State: APzg51Dj3I+RDXTzO99mPnggNod8FDP4E3gO/DJrE3PTi0xc5a4WlUUo GW545d/Mp4iauNbSfIH8HIWeRQ== X-Received: by 2002:a37:f9d:: with SMTP id 29-v6mr30189012qkp.163.1536086029434; Tue, 04 Sep 2018 11:33:49 -0700 (PDT) Received: from ?IPv6:2601:602:9802:a8dc::3c7a? ([2601:602:9802:a8dc::3c7a]) by smtp.gmail.com with ESMTPSA id p4-v6sm13947954qkl.41.2018.09.04.11.33.47 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 04 Sep 2018 11:33:48 -0700 (PDT) Subject: Re: [PATCH v2] staging: android: ion: fix ION_IOC_{MAP,SHARE} use-after-free To: Greg Hackmann , linux-kernel@vger.kernel.org Cc: Sumit Semwal , Greg Kroah-Hartman , devel@driverdev.osuosl.org, kernel-team@android.com, Greg Hackmann , stable@vger.kernel.org References: <20180904163336.234819-1-ghackmann@google.com> From: Laura Abbott Message-ID: Date: Tue, 4 Sep 2018 11:33:46 -0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20180904163336.234819-1-ghackmann@google.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 09/04/2018 09:33 AM, Greg Hackmann wrote: > The ION_IOC_{MAP,SHARE} ioctls drop and reacquire client->lock several > times while operating on one of the client's ion_handles. This creates > windows where userspace can call ION_IOC_FREE on the same client with > the same handle, and effectively make the kernel drop its own reference. > For example: > > - thread A: ION_IOC_ALLOC creates an ion_handle with refcount 1 > - thread A: starts ION_IOC_MAP and increments the refcount to 2 > - thread B: ION_IOC_FREE decrements the refcount to 1 > - thread B: ION_IOC_FREE decrements the refcount to 0 and frees the > handle > - thread A: continues ION_IOC_MAP with a dangling ion_handle * to > freed memory > > Fix this by holding client->lock for the duration of > ION_IOC_{MAP,SHARE}, preventing the concurrent ION_IOC_FREE. Also > remove ion_handle_get_by_id(), since there's literally no way to use it > safely. > > This patch is applied on top of 4.4.y, and applies to older kernels > too. 4.9.y was fixed separately. Kernels 4.12 and later are > unaffected, since all the underlying ion_handle infrastructure has been > ripped out. > Acked-by: Laura Abbott > Cc: stable@vger.kernel.org # v4.4- > Signed-off-by: Greg Hackmann > --- > v2: remove Change-Id line from commit message > > drivers/staging/android/ion/ion.c | 60 +++++++++++++++++++------------ > 1 file changed, 37 insertions(+), 23 deletions(-) > > diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c > index 47cb163da9a0..4adb1138af09 100644 > --- a/drivers/staging/android/ion/ion.c > +++ b/drivers/staging/android/ion/ion.c > @@ -449,18 +449,6 @@ static struct ion_handle *ion_handle_get_by_id_nolock(struct ion_client *client, > return ERR_PTR(-EINVAL); > } > > -struct ion_handle *ion_handle_get_by_id(struct ion_client *client, > - int id) > -{ > - struct ion_handle *handle; > - > - mutex_lock(&client->lock); > - handle = ion_handle_get_by_id_nolock(client, id); > - mutex_unlock(&client->lock); > - > - return handle; > -} > - > static bool ion_handle_validate(struct ion_client *client, > struct ion_handle *handle) > { > @@ -1138,24 +1126,28 @@ static struct dma_buf_ops dma_buf_ops = { > .kunmap = ion_dma_buf_kunmap, > }; > > -struct dma_buf *ion_share_dma_buf(struct ion_client *client, > - struct ion_handle *handle) > +static struct dma_buf *__ion_share_dma_buf(struct ion_client *client, > + struct ion_handle *handle, > + bool lock_client) > { > DEFINE_DMA_BUF_EXPORT_INFO(exp_info); > struct ion_buffer *buffer; > struct dma_buf *dmabuf; > bool valid_handle; > > - mutex_lock(&client->lock); > + if (lock_client) > + mutex_lock(&client->lock); > valid_handle = ion_handle_validate(client, handle); > if (!valid_handle) { > WARN(1, "%s: invalid handle passed to share.\n", __func__); > - mutex_unlock(&client->lock); > + if (lock_client) > + mutex_unlock(&client->lock); > return ERR_PTR(-EINVAL); > } > buffer = handle->buffer; > ion_buffer_get(buffer); > - mutex_unlock(&client->lock); > + if (lock_client) > + mutex_unlock(&client->lock); > > exp_info.ops = &dma_buf_ops; > exp_info.size = buffer->size; > @@ -1170,14 +1162,21 @@ struct dma_buf *ion_share_dma_buf(struct ion_client *client, > > return dmabuf; > } > + > +struct dma_buf *ion_share_dma_buf(struct ion_client *client, > + struct ion_handle *handle) > +{ > + return __ion_share_dma_buf(client, handle, true); > +} > EXPORT_SYMBOL(ion_share_dma_buf); > > -int ion_share_dma_buf_fd(struct ion_client *client, struct ion_handle *handle) > +static int __ion_share_dma_buf_fd(struct ion_client *client, > + struct ion_handle *handle, bool lock_client) > { > struct dma_buf *dmabuf; > int fd; > > - dmabuf = ion_share_dma_buf(client, handle); > + dmabuf = __ion_share_dma_buf(client, handle, lock_client); > if (IS_ERR(dmabuf)) > return PTR_ERR(dmabuf); > > @@ -1187,8 +1186,19 @@ int ion_share_dma_buf_fd(struct ion_client *client, struct ion_handle *handle) > > return fd; > } > + > +int ion_share_dma_buf_fd(struct ion_client *client, struct ion_handle *handle) > +{ > + return __ion_share_dma_buf_fd(client, handle, true); > +} > EXPORT_SYMBOL(ion_share_dma_buf_fd); > > +static int ion_share_dma_buf_fd_nolock(struct ion_client *client, > + struct ion_handle *handle) > +{ > + return __ion_share_dma_buf_fd(client, handle, false); > +} > + > struct ion_handle *ion_import_dma_buf(struct ion_client *client, int fd) > { > struct dma_buf *dmabuf; > @@ -1335,11 +1345,15 @@ static long ion_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) > { > struct ion_handle *handle; > > - handle = ion_handle_get_by_id(client, data.handle.handle); > - if (IS_ERR(handle)) > + mutex_lock(&client->lock); > + handle = ion_handle_get_by_id_nolock(client, data.handle.handle); > + if (IS_ERR(handle)) { > + mutex_unlock(&client->lock); > return PTR_ERR(handle); > - data.fd.fd = ion_share_dma_buf_fd(client, handle); > - ion_handle_put(handle); > + } > + data.fd.fd = ion_share_dma_buf_fd_nolock(client, handle); > + ion_handle_put_nolock(handle); > + mutex_unlock(&client->lock); > if (data.fd.fd < 0) > ret = data.fd.fd; > break; >