linux-kernel - Re: [PATCH for-6.8 v3 1/3] LoongArch: KVM: Fix input validation of _kvm_get_cpucfg and kvm_check

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Thu, 22 Feb 2024 18:22:42 +0800
From: maobibo <maobibo@...ngson.cn>
To: WANG Xuerui <kernel@...0n.name>, Paolo Bonzini <pbonzini@...hat.com>,
 Huacai Chen <chenhuacai@...nel.org>
Cc: Tianrui Zhao <zhaotianrui@...ngson.cn>, kvm@...r.kernel.org,
 loongarch@...ts.linux.dev, linux-kernel@...r.kernel.org,
 WANG Xuerui <git@...0n.name>
Subject: Re: [PATCH for-6.8 v3 1/3] LoongArch: KVM: Fix input validation of
 _kvm_get_cpucfg and kvm_check_cpucfg



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