Received-SPF: pass (google.com: domain of linux-kernel+bounces-76316-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Subject: Re: [PATCH for-6.8 v3 1/3] LoongArch: KVM: Fix input validation of
 _kvm_get_cpucfg and kvm_check_cpucfg
To: WANG Xuerui <kernel@xen0n.name>, Paolo Bonzini <pbonzini@redhat.com>,
 Huacai Chen <chenhuacai@kernel.org>
Cc: Tianrui Zhao <zhaotianrui@loongson.cn>, kvm@vger.kernel.org,
 loongarch@lists.linux.dev, linux-kernel@vger.kernel.org,
 WANG Xuerui <git@xen0n.name>
References: <20240216085822.3032984-1-kernel@xen0n.name>
 <20240216085822.3032984-2-kernel@xen0n.name>
 <412ea29b-7a53-1f91-1cdb-5a256e74826b@loongson.cn>
 <4a12394a-8ebf-40b8-b0bc-65b5a66967cd@xen0n.name>
From: maobibo <maobibo@loongson.cn>
Message-ID: <0c27477b-d144-37f3-d47c-956f9ba07723@loongson.cn>
Date: Thu, 22 Feb 2024 18:22:42 +0800
User-Agent: Mozilla/5.0 (X11; Linux loongarch64; rv:68.0) Gecko/20100101
 Thunderbird/68.7.0
Precedence: bulk
MIME-Version: 1.0
In-Reply-To: <4a12394a-8ebf-40b8-b0bc-65b5a66967cd@xen0n.name>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 8bit


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.
>