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

[maobibo]

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.