[for-6.8,v3,1/3] LoongArch: KVM: Fix input validation of _kvm_get_cpucfg and kvm_check_cpucfg
Message ID | 20240216085822.3032984-2-kernel@xen0n.name |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel+bounces-68291-ouuuleilei=gmail.com@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a05:7300:c619:b0:108:e6aa:91d0 with SMTP id hn25csp384049dyb; Fri, 16 Feb 2024 01:00:19 -0800 (PST) X-Forwarded-Encrypted: i=3; AJvYcCXKAl7UaTXkoSFi6xXq58LN/dJxRZIo2zpbh3TUYfN7BIxfnJj78qyUd3NOL0WElxOJn2a5ZdT7JfWJaXin7BZRoDghUw== X-Google-Smtp-Source: AGHT+IGWSGrZj630FsWPvakf/67v7t07U4zBwQlwoVKbp241V6nbWNoDBkkg1ovHXPI6nyfMUJEH X-Received: by 2002:a05:6a20:9f48:b0:19e:4c37:8737 with SMTP id ml8-20020a056a209f4800b0019e4c378737mr5559457pzb.5.1708074019347; Fri, 16 Feb 2024 01:00:19 -0800 (PST) ARC-Seal: i=2; a=rsa-sha256; t=1708074019; cv=pass; d=google.com; s=arc-20160816; b=zZATqX4WjOL3ZmA92gsGzNxJ/tk+KqugE1xRbqWB/53UrVa2k2ufa8lKs5hWwqeFvS lF/RtfpK24RAlkjZ/NnueiKGJGXGmNKdwgUdVoMb5HENJHlD0SPPnVhtPDjVwSKIZBpt swA/1vblL8b6iOZ0dhPCSOi6pkI4PfAF4j76GIZylTkCrrpFuWFati4GgBahVBHGrYl+ GIgOvCUuAwa8+YncI2mXJUjPHIMobaRShZIC/tjpvOcIVJ6+ylSGtCKaAo4K5iv6G9Uj uw2Pk1mmWJRW0wFDRzxsMJWTol+X2r0rT0H+MC6uV3RIDdmEFTfiv2xiD4oGsK+I0nB/ cgZQ== ARC-Message-Signature: i=2; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=content-transfer-encoding:mime-version:list-unsubscribe :list-subscribe:list-id:precedence:references:in-reply-to:message-id :date:subject:cc:to:from:dkim-signature; bh=/xcLlUewoDSyqmj+ThTqR0RScb4XcIL6eaBf8LnScPY=; fh=mBf034bH8hBnsBxAupk5xQQrUQuaJjqEyteqgdDSbfg=; b=uGMBG89+KyvD3JzoeBmafcfbl3vkzFog47MEQDBqRgj3mQWBcECcMMwRyFONlmtXUx fojBIxclxaz+BZlgCf9jlqca2kn3DzPQS9OjA8p23ygKFYVuP7IEsY3qGYMu6b9obImK gHD8zy2q1c4ZoVp3DxFkT0znLryJjEBMa2mO8DxXxVy2beQwdnnGQVVlsrGLCGiHocCU fsomB6XfNyGOYLZA4J10OKY6uBBo+6Bl244I7k3LkTrkKp221B53Kentdj+JudCjBBKL QFwpL2z8xBqZ1BkZh1vP70HcTMV91eGWVqlB7seacpP2oHIiH78fbJH7Hwtg4wyXpMaM E6WA==; dara=google.com ARC-Authentication-Results: i=2; mx.google.com; dkim=pass header.i=@xen0n.name header.s=mail header.b=mO76rEvU; arc=pass (i=1 spf=pass spfdomain=xen0n.name dkim=pass dkdomain=xen0n.name); spf=pass (google.com: domain of linux-kernel+bounces-68291-ouuuleilei=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) smtp.mailfrom="linux-kernel+bounces-68291-ouuuleilei=gmail.com@vger.kernel.org" Received: from sv.mirrors.kernel.org (sv.mirrors.kernel.org. [2604:1380:45e3:2400::1]) by mx.google.com with ESMTPS id q27-20020aa7983b000000b006e04030ef0dsi2650334pfl.195.2024.02.16.01.00.19 for <ouuuleilei@gmail.com> (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 16 Feb 2024 01:00:19 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel+bounces-68291-ouuuleilei=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1; Authentication-Results: mx.google.com; dkim=pass header.i=@xen0n.name header.s=mail header.b=mO76rEvU; arc=pass (i=1 spf=pass spfdomain=xen0n.name dkim=pass dkdomain=xen0n.name); spf=pass (google.com: domain of linux-kernel+bounces-68291-ouuuleilei=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) smtp.mailfrom="linux-kernel+bounces-68291-ouuuleilei=gmail.com@vger.kernel.org" Received: from smtp.subspace.kernel.org (wormhole.subspace.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by sv.mirrors.kernel.org (Postfix) with ESMTPS id 2490828A89B for <ouuuleilei@gmail.com>; Fri, 16 Feb 2024 09:00:19 +0000 (UTC) Received: from localhost.localdomain (localhost.localdomain [127.0.0.1]) by smtp.subspace.kernel.org (Postfix) with ESMTP id 9DDE61CAA8; Fri, 16 Feb 2024 08:58:40 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=xen0n.name header.i=@xen0n.name header.b="mO76rEvU" Received: from mailbox.box.xen0n.name (mail.xen0n.name [115.28.160.31]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id BB6571B95D; Fri, 16 Feb 2024 08:58:35 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=115.28.160.31 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1708073917; cv=none; b=QjcESJ9GvFtq9h1QwuyCODzJz1MyLKUO26X4twDixOTbZwJPdv8fkT4qDRgyWFPB0S0vdrcyeaMPY5SoDLPBi1LlU1AKDDZbp9zr4rXlPFu9z7lc4xvxsUvUUjRu3308rHESriz1Kn0+XhcDBOWiUUssPpzeNfMFR+QbzpdkzrE= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1708073917; c=relaxed/simple; bh=NcJCcfqi8ekicRsCx7lcKlZxsG+ADiWoan5/EkMI1NU=; h=From:To:Cc:Subject:Date:Message-ID:In-Reply-To:References: MIME-Version; b=SXhUAfPp98vVdw/1QA/6CmOJe/PuJs6mtx40CnqDJ+oM3JUCQmKE996vcuKKJUhOf1ciZZwIKpIOtN9OLpMQJzAYAj22mt5FZZjI5QA8+Vq4uPCI4t+X2+SUNR2jMPBXejf93ZPja+Gv389hajSIWUjubncDmVBUc9A7fm5CYUk= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=xen0n.name; spf=pass smtp.mailfrom=xen0n.name; dkim=pass (1024-bit key) header.d=xen0n.name header.i=@xen0n.name header.b=mO76rEvU; arc=none smtp.client-ip=115.28.160.31 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=xen0n.name Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=xen0n.name DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=xen0n.name; s=mail; t=1708073910; bh=NcJCcfqi8ekicRsCx7lcKlZxsG+ADiWoan5/EkMI1NU=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=mO76rEvUz/01p234o2SAAs2to05wG3+DNg3sb00mT0knnk5q422OQL/bhqv+NbnSe I5Kd9N/o4VZ4UPQQ6BlhBp+z9TWsXqViVMaQxsB7VxmdZBpH04C5P9qfhtiHmz3rjk vBcMsrblEov56rBFalDCYfWXXLysYyfeeUvL7QvI= Received: from ld50.lan (unknown [IPv6:240e:388:8d00:6500:cda4:aa27:b0f6:1748]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mailbox.box.xen0n.name (Postfix) with ESMTPSA id 21BF760187; Fri, 16 Feb 2024 16:58:30 +0800 (CST) From: WANG Xuerui <kernel@xen0n.name> To: Paolo Bonzini <pbonzini@redhat.com>, Huacai Chen <chenhuacai@kernel.org> Cc: Tianrui Zhao <zhaotianrui@loongson.cn>, Bibo Mao <maobibo@loongson.cn>, kvm@vger.kernel.org, loongarch@lists.linux.dev, linux-kernel@vger.kernel.org, WANG Xuerui <git@xen0n.name> Subject: [PATCH for-6.8 v3 1/3] LoongArch: KVM: Fix input validation of _kvm_get_cpucfg and kvm_check_cpucfg Date: Fri, 16 Feb 2024 16:58:20 +0800 Message-ID: <20240216085822.3032984-2-kernel@xen0n.name> X-Mailer: git-send-email 2.43.0 In-Reply-To: <20240216085822.3032984-1-kernel@xen0n.name> References: <20240216085822.3032984-1-kernel@xen0n.name> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: <linux-kernel.vger.kernel.org> List-Subscribe: <mailto:linux-kernel+subscribe@vger.kernel.org> List-Unsubscribe: <mailto:linux-kernel+unsubscribe@vger.kernel.org> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1791045423214109203 X-GMAIL-MSGID: 1791045423214109203 |
Series |
KVM: LoongArch: Fix wrong CPUCFG ID handling
|
|
Commit Message
WANG Xuerui
Feb. 16, 2024, 8:58 a.m. UTC
From: WANG Xuerui <git@xen0n.name> The range check for the CPUCFG ID is wrong (should have been a || instead of &&); it is conceptually simpler to just express the check as another case of the switch statement on the ID. As it turns out to be the case, the userland (currently only the QEMU/KVM target code) expects to set CPUCFG IDs 0 to 20 inclusive, but only CPUCFG2 values are being validated. Furthermore, the juggling of the temp return value is unnecessary, because it is semantically equivalent and more readable to just return at every switch case's end. This is done too to avoid potential bugs in the future related to the unwanted complexity. Also, the return value of _kvm_get_cpucfg is meant to be checked, but this was not done, so bad CPUCFG IDs wrongly fall back to the default case and 0 is incorrectly returned; check the return value to fix the UAPI behavior. While at it, also remove the redundant range check in kvm_check_cpucfg, because out-of-range CPUCFG IDs are already rejected by the -EINVAL as returned by _kvm_get_cpucfg. Fixes: db1ecca22edf ("LoongArch: KVM: Add LSX (128bit SIMD) support") Signed-off-by: WANG Xuerui <git@xen0n.name> --- arch/loongarch/kvm/vcpu.c | 33 +++++++++++++++------------------ 1 file changed, 15 insertions(+), 18 deletions(-)
Comments
Hi Xuerui, Good catch, and thank for your patch. On 2024/2/16 下午4:58, WANG Xuerui wrote: > From: WANG Xuerui <git@xen0n.name> > > The range check for the CPUCFG ID is wrong (should have been a || > instead of &&); it is conceptually simpler to just express the check > as another case of the switch statement on the ID. As it turns out to be > the case, the userland (currently only the QEMU/KVM target code) expects > to set CPUCFG IDs 0 to 20 inclusive, but only CPUCFG2 values are being > validated. > > Furthermore, the juggling of the temp return value is unnecessary, > because it is semantically equivalent and more readable to just > return at every switch case's end. This is done too to avoid potential > bugs in the future related to the unwanted complexity. > > Also, the return value of _kvm_get_cpucfg is meant to be checked, but > this was not done, so bad CPUCFG IDs wrongly fall back to the default > case and 0 is incorrectly returned; check the return value to fix the > UAPI behavior. > > While at it, also remove the redundant range check in kvm_check_cpucfg, > because out-of-range CPUCFG IDs are already rejected by the -EINVAL > as returned by _kvm_get_cpucfg. > > Fixes: db1ecca22edf ("LoongArch: KVM: Add LSX (128bit SIMD) support") > Signed-off-by: WANG Xuerui <git@xen0n.name> > --- > arch/loongarch/kvm/vcpu.c | 33 +++++++++++++++------------------ > 1 file changed, 15 insertions(+), 18 deletions(-) > > diff --git a/arch/loongarch/kvm/vcpu.c b/arch/loongarch/kvm/vcpu.c > index 27701991886d..56da0881fc94 100644 > --- a/arch/loongarch/kvm/vcpu.c > +++ b/arch/loongarch/kvm/vcpu.c > @@ -300,11 +300,6 @@ static int _kvm_setcsr(struct kvm_vcpu *vcpu, unsigned int id, u64 val) > > static int _kvm_get_cpucfg(int id, u64 *v) > { > - int ret = 0; > - > - if (id < 0 && id >= KVM_MAX_CPUCFG_REGS) > - return -EINVAL; > - yes, it had better be removed since the same thing is done in function kvm_check_cpucfg(). > switch (id) { > case 2: > /* Return CPUCFG2 features which have been supported by KVM */ > @@ -324,31 +319,33 @@ static int _kvm_get_cpucfg(int id, u64 *v) > if (cpu_has_lasx) > *v |= CPUCFG2_LASX; > > - break; > + return 0; > + case 0 ... 1: > + case 3 ... KVM_MAX_CPUCFG_REGS - 1: > + /* no restrictions on other CPUCFG IDs' values */ > + *v = U64_MAX; > + return 0; how about something like this? default: /* no restrictions on other CPUCFG IDs' values */ *v = U64_MAX; return 0; Regards Bibo Mao > default: > - ret = -EINVAL; > - break; > + return -EINVAL; > } > - return ret; > } > > static int kvm_check_cpucfg(int id, u64 val) > { > - u64 mask; > - int ret = 0; > - > - if (id < 0 && id >= KVM_MAX_CPUCFG_REGS) > - return -EINVAL; > + u64 mask = 0; > + int ret; > > - if (_kvm_get_cpucfg(id, &mask)) > + ret = _kvm_get_cpucfg(id, &mask); > + if (ret) > return ret; > > + if (val & ~mask) > + /* Unsupported features should not be set */ > + return -EINVAL; > + > switch (id) { > case 2: > /* CPUCFG2 features checking */ > - if (val & ~mask) > - /* The unsupported features should not be set */ > - ret = -EINVAL; > else if (!(val & CPUCFG2_LLFTP)) > /* The LLFTP must be set, as guest must has a constant timer */ > ret = -EINVAL; >
Hi, On 2/17/24 11:03, maobibo wrote: > Hi Xuerui, > > Good catch, and thank for your patch. > > On 2024/2/16 下午4:58, WANG Xuerui wrote: >> [snip] >> @@ -324,31 +319,33 @@ static int _kvm_get_cpucfg(int id, u64 *v) >> if (cpu_has_lasx) >> *v |= CPUCFG2_LASX; >> - break; >> + return 0; >> + case 0 ... 1: >> + case 3 ... KVM_MAX_CPUCFG_REGS - 1: >> + /* no restrictions on other CPUCFG IDs' values */ >> + *v = U64_MAX; >> + return 0; > how about something like this? > default: > /* no restrictions on other CPUCFG IDs' values */ > *v = U64_MAX; > return 0; I don't think this version correctly expresses the intent. Note that the CPUCFG ID range check is squashed into the switch as well, so one switch conveniently expresses the three intended cases at once: * the special treatment of CPUCFG2, * all-allow rules for other in-range CPUCFG IDs, and * rejection for out-of-range IDs. Yet the suggestion here is conflating the latter two cases, with the effect of allowing every ID that's not 2 to take any value (as expressed by the U64_MAX mask), and *removing the range check* (because no return path returns -EINVAL with this change). So I'd like to stick to the current version, but thanks anyway for your kind review and suggestion.
On 2024/2/22 下午5:45, WANG Xuerui wrote: > Hi, > > On 2/17/24 11:03, maobibo wrote: >> Hi Xuerui, >> >> Good catch, and thank for your patch. >> >> On 2024/2/16 下午4:58, WANG Xuerui wrote: >>> [snip] >>> @@ -324,31 +319,33 @@ static int _kvm_get_cpucfg(int id, u64 *v) >>> if (cpu_has_lasx) >>> *v |= CPUCFG2_LASX; >>> - break; >>> + return 0; >>> + case 0 ... 1: >>> + case 3 ... KVM_MAX_CPUCFG_REGS - 1: >>> + /* no restrictions on other CPUCFG IDs' values */ >>> + *v = U64_MAX; >>> + return 0; >> how about something like this? >> default: >> /* no restrictions on other CPUCFG IDs' values */ >> *v = U64_MAX; >> return 0; > > I don't think this version correctly expresses the intent. Note that the > CPUCFG ID range check is squashed into the switch as well, so one switch > conveniently expresses the three intended cases at once: > > * the special treatment of CPUCFG2, + case 0 ... 1: + case 3 ... KVM_MAX_CPUCFG_REGS - 1: + /* no restrictions on other CPUCFG IDs' values */ + *v = U64_MAX; + return 0; cpucfg6 checking will be added for PMU support soon. So it will be case 6: do something check for cpucfg6 return mask; case 0 ... 1: case 3 ... 5: case 7 ... KVM_MAX_CPUCFG_REGS - 1: *v = U64_MAX; return 0; If you think it is reasonable to add these separate "case" sentences, I have no objection. > * all-allow rules for other in-range CPUCFG IDs, and > * rejection for out-of-range IDs. static int kvm_check_cpucfg(int id, u64 val) { - u64 mask; - int ret = 0; - - if (id < 0 && id >= KVM_MAX_CPUCFG_REGS) - return -EINVAL; you can modify && with ||, like this: if (id < 0 || id >= KVM_MAX_CPUCFG_REGS) return -EINVAL; + u64 mask = 0; + int ret; Regards Bibo Mao > > Yet the suggestion here is conflating the latter two cases, with the > effect of allowing every ID that's not 2 to take any value (as expressed > by the U64_MAX mask), and *removing the range check* (because no return > path returns -EINVAL with this change). > > So I'd like to stick to the current version, but thanks anyway for your > kind review and suggestion. >
On 2/22/24 18:22, maobibo wrote: > > > On 2024/2/22 下午5:45, WANG Xuerui wrote: >> Hi, >> >> On 2/17/24 11:03, maobibo wrote: >>> Hi Xuerui, >>> >>> Good catch, and thank for your patch. >>> >>> On 2024/2/16 下午4:58, WANG Xuerui wrote: >>>> [snip] >>>> @@ -324,31 +319,33 @@ static int _kvm_get_cpucfg(int id, u64 *v) >>>> if (cpu_has_lasx) >>>> *v |= CPUCFG2_LASX; >>>> - break; >>>> + return 0; >>>> + case 0 ... 1: >>>> + case 3 ... KVM_MAX_CPUCFG_REGS - 1: >>>> + /* no restrictions on other CPUCFG IDs' values */ >>>> + *v = U64_MAX; >>>> + return 0; >>> how about something like this? >>> default: >>> /* no restrictions on other CPUCFG IDs' values */ >>> *v = U64_MAX; >>> return 0; >> >> I don't think this version correctly expresses the intent. Note that >> the CPUCFG ID range check is squashed into the switch as well, so one >> switch conveniently expresses the three intended cases at once: >> >> * the special treatment of CPUCFG2, > + case 0 ... 1: > + case 3 ... KVM_MAX_CPUCFG_REGS - 1: > + /* no restrictions on other CPUCFG IDs' values */ > + *v = U64_MAX; > + return 0; > cpucfg6 checking will be added for PMU support soon. So it will be > case 6: > do something check for cpucfg6 > return mask; > case 0 ... 1: > case 3 ... 5: > case 7 ... KVM_MAX_CPUCFG_REGS - 1: > *v = U64_MAX; > return 0; > > If you think it is reasonable to add these separate "case" sentences, I > have no objection. >> * all-allow rules for other in-range CPUCFG IDs, and >> * rejection for out-of-range IDs. > static int kvm_check_cpucfg(int id, u64 val) > { > - u64 mask; > - int ret = 0; > - > - if (id < 0 && id >= KVM_MAX_CPUCFG_REGS) > - return -EINVAL; > you can modify && with ||, like this: > if (id < 0 || id >= KVM_MAX_CPUCFG_REGS) > return -EINVAL; Yes I know. Personally I don't find the "three cases" that annoying, but I agree with you that it can be mildly frustrating if one case addition would split the ranges even more. Given I'd like to also change the U64_MAX into U32_MAX (CPUCFG data is specified to be 32-bit wide), I'll send a v4 that keeps the "if" branch to make more people happy (Huacai privately also expressed preference for minimizing changes to the overall code shape). Thanks for your suggestion.
diff --git a/arch/loongarch/kvm/vcpu.c b/arch/loongarch/kvm/vcpu.c index 27701991886d..56da0881fc94 100644 --- a/arch/loongarch/kvm/vcpu.c +++ b/arch/loongarch/kvm/vcpu.c @@ -300,11 +300,6 @@ static int _kvm_setcsr(struct kvm_vcpu *vcpu, unsigned int id, u64 val) static int _kvm_get_cpucfg(int id, u64 *v) { - int ret = 0; - - if (id < 0 && id >= KVM_MAX_CPUCFG_REGS) - return -EINVAL; - switch (id) { case 2: /* Return CPUCFG2 features which have been supported by KVM */ @@ -324,31 +319,33 @@ static int _kvm_get_cpucfg(int id, u64 *v) if (cpu_has_lasx) *v |= CPUCFG2_LASX; - break; + return 0; + case 0 ... 1: + case 3 ... KVM_MAX_CPUCFG_REGS - 1: + /* no restrictions on other CPUCFG IDs' values */ + *v = U64_MAX; + return 0; default: - ret = -EINVAL; - break; + return -EINVAL; } - return ret; } static int kvm_check_cpucfg(int id, u64 val) { - u64 mask; - int ret = 0; - - if (id < 0 && id >= KVM_MAX_CPUCFG_REGS) - return -EINVAL; + u64 mask = 0; + int ret; - if (_kvm_get_cpucfg(id, &mask)) + ret = _kvm_get_cpucfg(id, &mask); + if (ret) return ret; + if (val & ~mask) + /* Unsupported features should not be set */ + return -EINVAL; + switch (id) { case 2: /* CPUCFG2 features checking */ - if (val & ~mask) - /* The unsupported features should not be set */ - ret = -EINVAL; else if (!(val & CPUCFG2_LLFTP)) /* The LLFTP must be set, as guest must has a constant timer */ ret = -EINVAL;