From patchwork Wed Feb 21 17:49:10 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Al Viro X-Patchwork-Id: 10233657 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 53AD260209 for ; Wed, 21 Feb 2018 17:49:26 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 48FB826E47 for ; Wed, 21 Feb 2018 17:49:26 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 3CDDE27E71; Wed, 21 Feb 2018 17:49:26 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.9 required=2.0 tests=BAYES_00,RCVD_IN_DNSWL_HI autolearn=unavailable version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id C795226E47 for ; Wed, 21 Feb 2018 17:49:25 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755003AbeBURtP (ORCPT ); Wed, 21 Feb 2018 12:49:15 -0500 Received: from zeniv.linux.org.uk ([195.92.253.2]:54372 "EHLO ZenIV.linux.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S966069AbeBURtN (ORCPT ); Wed, 21 Feb 2018 12:49:13 -0500 Received: from viro by ZenIV.linux.org.uk with local (Exim 4.87 #1 (Red Hat Linux)) id 1eoYVy-0007iB-EM; Wed, 21 Feb 2018 17:49:10 +0000 Date: Wed, 21 Feb 2018 17:49:10 +0000 From: Al Viro To: Brijesh Singh Cc: kvm@vger.kernel.org, Paolo Bonzini , Radim =?utf-8?B?S3LEjW3DocWZ?= , Borislav Petkov , Tom Lendacky , linux-kernel@vger.kernel.org, Joerg Roedel Subject: Re: [PATCH] KVM: SVM: Fix sparse: incorrect type in argument 1 (different base types) Message-ID: <20180221174910.GI30522@ZenIV.linux.org.uk> References: <20180219161228.46931-1-brijesh.singh@amd.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20180219161228.46931-1-brijesh.singh@amd.com> User-Agent: Mutt/1.9.1 (2017-09-22) Sender: kvm-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: kvm@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Mon, Feb 19, 2018 at 10:12:28AM -0600, Brijesh Singh wrote: > Fix sparse: incorrect type in argument 1 (different base types). Typecast > the userspace address argument. Better question: why the hell do we want that access_ok(), anyway? The only thing we do to params.uaddr is if (blob) { if (copy_to_user((void __user *)(uintptr_t)params.uaddr, blob, params.len)) ret = -EFAULT; } downstream. What does that access_ok() buy us? It does not guarantee that copy_to_user() won't fail. It does not clamp params.len (we'd just done that explicitly). So why not somethings like this: diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c index b3e488a74828..ba2c1a606985 100644 --- a/arch/x86/kvm/svm.c +++ b/arch/x86/kvm/svm.c @@ -6239,13 +6239,15 @@ static int sev_launch_measure(struct kvm *kvm, struct kvm_sev_cmd *argp) struct kvm_sev_info *sev = &kvm->arch.sev_info; struct sev_data_launch_measure *data; struct kvm_sev_launch_measure params; + void __user *measure = (void __user *)(uintptr_t)argp->data; + void __user *p = NULL; void *blob = NULL; int ret; if (!sev_guest(kvm)) return -ENOTTY; - if (copy_from_user(¶ms, (void __user *)(uintptr_t)argp->data, sizeof(params))) + if (copy_from_user(¶ms, measure, sizeof(params))) return -EFAULT; data = kzalloc(sizeof(*data), GFP_KERNEL); @@ -6256,17 +6258,13 @@ static int sev_launch_measure(struct kvm *kvm, struct kvm_sev_cmd *argp) if (!params.len) goto cmd; - if (params.uaddr) { + p = (void __user *)(uintptr_t)params.uaddr; + if (p) { if (params.len > SEV_FW_BLOB_MAX_SIZE) { ret = -EINVAL; goto e_free; } - if (!access_ok(VERIFY_WRITE, params.uaddr, params.len)) { - ret = -EFAULT; - goto e_free; - } - ret = -ENOMEM; blob = kmalloc(params.len, GFP_KERNEL); if (!blob) @@ -6290,13 +6288,13 @@ static int sev_launch_measure(struct kvm *kvm, struct kvm_sev_cmd *argp) goto e_free_blob; if (blob) { - if (copy_to_user((void __user *)(uintptr_t)params.uaddr, blob, params.len)) + if (copy_to_user(p, blob, params.len)) ret = -EFAULT; } done: params.len = data->len; - if (copy_to_user((void __user *)(uintptr_t)argp->data, ¶ms, sizeof(params))) + if (copy_to_user(measure, ¶ms, sizeof(params))) ret = -EFAULT; e_free_blob: kfree(blob);