[BUG] 6.10 stable: arch/x86/kvm/xen.c:1486:44: error: use of uninitialized value ‘port’ [CWE-457]

[BUG] 6.10 stable: arch/x86/kvm/xen.c:1486:44: error: use of uninitialized value ‘port’ [CWE-457]

[Mirsad Todorovac]

Hi, all,

While building stable tree version of 6.10, the following error occurred:

In line 1421 defines:

1421        evtchn_port_t port, *ports;

The ports becomes &port in line 1470, but neither port nor *ports is assigned a value
until line 1486 where port is used:

1485         if (sched_poll.nr_ports == 1)
1486 →               vcpu->arch.xen.poll_evtchn = port;

The visual inspection proves that the compiler is again right (GCC 12.3.0).

The linux-next and kvm trees contained the same error.

In line 1507 this error is rectified by setting vcpu->arch.xen.poll_evtchn = 0,
but compiler still prevents build with -Werror.

I don't have familiarity with this section of code.

"arch/x86/kvm/xen.c"
--------------------
1417 static bool kvm_xen_schedop_poll(struct kvm_vcpu *vcpu, bool longmode,
1418                                  u64 param, u64 *r)
1419 {
1420         struct sched_poll sched_poll;
1421 →       evtchn_port_t port, *ports;
1422         struct x86_exception e;
1423         int i;
1424 
1425         if (!lapic_in_kernel(vcpu) ||
1426             !(vcpu->kvm->arch.xen_hvm_config.flags & KVM_XEN_HVM_CONFIG_EVTCHN_SEND))
1427                 return false;
1428 
1429         if (IS_ENABLED(CONFIG_64BIT) && !longmode) {
1430                 struct compat_sched_poll sp32;
1431 
1432                 /* Sanity check that the compat struct definition is correct */
1433                 BUILD_BUG_ON(sizeof(sp32) != 16);
1434 
1435                 if (kvm_read_guest_virt(vcpu, param, &sp32, sizeof(sp32), &e)) {
1436                         *r = -EFAULT;
1437                         return true;
1438                 }
1439 
1440                 /*
1441                  * This is a 32-bit pointer to an array of evtchn_port_t which
1442                  * are uint32_t, so once it's converted no further compat
1443                  * handling is needed.
1444                  */
1445                 sched_poll.ports = (void *)(unsigned long)(sp32.ports);
1446                 sched_poll.nr_ports = sp32.nr_ports;
1447                 sched_poll.timeout = sp32.timeout;
1448         } else {
1449                 if (kvm_read_guest_virt(vcpu, param, &sched_poll,
1450                                         sizeof(sched_poll), &e)) {
1451                         *r = -EFAULT;
1452                         return true;
1453                 }
1454         }
1455 
1456         if (unlikely(sched_poll.nr_ports > 1)) {
1457                 /* Xen (unofficially) limits number of pollers to 128 */
1458                 if (sched_poll.nr_ports > 128) {
1459                         *r = -EINVAL;
1460                         return true;
1461                 }
1462 
1463                 ports = kmalloc_array(sched_poll.nr_ports,
1464                                       sizeof(*ports), GFP_KERNEL);
1465                 if (!ports) {
1466                         *r = -ENOMEM;
1467                         return true;
1468                 }
1469         } else
1470 →                ports = &port;
1471 
1472         if (kvm_read_guest_virt(vcpu, (gva_t)sched_poll.ports, ports,
1473                                 sched_poll.nr_ports * sizeof(*ports), &e)) {
1474                 *r = -EFAULT;
1475                 return true;
1476         }
1477 
1478         for (i = 0; i < sched_poll.nr_ports; i++) {
1479                 if (ports[i] >= max_evtchn_port(vcpu->kvm)) {
1480                         *r = -EINVAL;
1481                         goto out;
1482                 }
1483         }
1484 
1485         if (sched_poll.nr_ports == 1)
1486 →               vcpu->arch.xen.poll_evtchn = port;
1487         else
1488                 vcpu->arch.xen.poll_evtchn = -1;
1489 
1490         set_bit(vcpu->vcpu_idx, vcpu->kvm->arch.xen.poll_mask);
1491 
1492         if (!wait_pending_event(vcpu, sched_poll.nr_ports, ports)) {
1493                 vcpu->arch.mp_state = KVM_MP_STATE_HALTED;
1494 
1495                 if (sched_poll.timeout)
1496                         mod_timer(&vcpu->arch.xen.poll_timer,
1497                                   jiffies + nsecs_to_jiffies(sched_poll.timeout));
1498 
1499                 kvm_vcpu_halt(vcpu);
1500 
1501                 if (sched_poll.timeout)
1502                         del_timer(&vcpu->arch.xen.poll_timer);
1503 
1504                 vcpu->arch.mp_state = KVM_MP_STATE_RUNNABLE;
1505         }
1506 
1507 →       vcpu->arch.xen.poll_evtchn = 0;
1508         *r = 0;
1509 out:
1510         /* Really, this is only needed in case of timeout */
1511         clear_bit(vcpu->vcpu_idx, vcpu->kvm->arch.xen.poll_mask);
1512 
1513         if (unlikely(sched_poll.nr_ports > 1))
1514                 kfree(ports);
1515         return true;
1516 }
--------------------

Hope this helps.

Best regards,
Mirsad Todorovac

Re: [BUG] 6.10 stable: arch/x86/kvm/xen.c:1486:44: error: use of uninitialized value ‘port’ [CWE-457]

[Sean Christopherson]

On Fri, Jul 19, 2024, Mirsad Todorovac wrote:
> Hi, all,
> 
> While building stable tree version of 6.10, the following error occurred:
> 
> In line 1421 defines:
> 
> 1421        evtchn_port_t port, *ports;
> 
> The ports becomes &port in line 1470, but neither port nor *ports is assigned a value
> until line 1486 where port is used:
> 
> 1485         if (sched_poll.nr_ports == 1)
> 1486 →               vcpu->arch.xen.poll_evtchn = port;
> 
> The visual inspection proves that the compiler is again right (GCC 12.3.0).

Nope, the compiler is wrong.  If sched_poll.nr_ports > 0, then kvm_read_guest_virt()
will fill ports[sched_poll.nr_ports].  If kvm_read_guest_virt() fails to do so,
it will return an error and the above code will never be reached.

	if (kvm_read_guest_virt(vcpu, (gva_t)sched_poll.ports, ports,
				sched_poll.nr_ports * sizeof(*ports), &e)) {
		*r = -EFAULT;
		return true;
	}

Re: [BUG] 6.10 stable: arch/x86/kvm/xen.c:1486:44: error: use of uninitialized value ‘port’ [CWE-457]

[Mirsad Todorovac]

On 7/19/24 19:30, Sean Christopherson wrote:
> On Fri, Jul 19, 2024, Mirsad Todorovac wrote:
>> Hi, all,
>>
>> While building stable tree version of 6.10, the following error occurred:
>>
>> In line 1421 defines:
>>
>> 1421        evtchn_port_t port, *ports;
>>
>> The ports becomes &port in line 1470, but neither port nor *ports is assigned a value
>> until line 1486 where port is used:
>>
>> 1485         if (sched_poll.nr_ports == 1)
>> 1486 →               vcpu->arch.xen.poll_evtchn = port;
>>
>> The visual inspection proves that the compiler is again right (GCC 12.3.0).
> 
> Nope, the compiler is wrong.  If sched_poll.nr_ports > 0, then kvm_read_guest_virt()
> will fill ports[sched_poll.nr_ports].  If kvm_read_guest_virt() fails to do so,
> it will return an error and the above code will never be reached.
> 
> 	if (kvm_read_guest_virt(vcpu, (gva_t)sched_poll.ports, ports,
> 				sched_poll.nr_ports * sizeof(*ports), &e)) {
> 		*r = -EFAULT;
> 		return true;
> 	}

Hi again,

I see what you mean.

Apparently, this dependency is not visible by the compiler, and this makes static analysers of
dubious value ...

I might need some advice on how to contribute to the Q/A of the kernel proper and testing suite
...

Best regards,
Mirsad Todorovac