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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.

--
WANG "xen0n" Xuerui

Linux/LoongArch mailing list: https://lore.kernel.org/loongarch/





[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux