Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] Resource leak in `hv_msi_domain_free()` when freeing multiple IRQs. - [High] Uninitialized memory write to hardware MSI registers in `hv_irq_compose_msi_msg()` error paths. - [High] Invalid hardware state read in `hv_build_devid_type_pci()` for Type 0 PCI-X leaf devices. --
commit 8c84576bf69ec6b4d58407408095bf71a9940bf2 Author: Mukesh R <[email protected]> x86/hyperv: Cosmetic changes in irqdomain.c for readability Make cosmetic changes such as renaming variables for consistency, fixing checkpatch warnings, updating indentation, and replacing spaces with tabs. > diff --git a/arch/x86/hyperv/irqdomain.c b/arch/x86/hyperv/irqdomain.c > --- a/arch/x86/hyperv/irqdomain.c > +++ b/arch/x86/hyperv/irqdomain.c [ ... ] > @@ -115,67 +112,71 @@ static int get_rid_cb(struct pci_dev *pdev, u16 alias, > void *data) > return 0; > } > > -static union hv_device_id hv_build_pci_dev_id(struct pci_dev *dev) > +static union hv_device_id hv_build_devid_type_pci(struct pci_dev *pdev) > { [ ... ] > - if (data.bridge) { > - int pos; > + if (data.bridge == NULL) > + goto out; > > - /* > - * Microsoft Hypervisor requires a bus range when the bridge is > - * running in PCI-X mode. [ ... ] > - pos = pci_find_capability(data.bridge, PCI_CAP_ID_PCIX); > - if (pos) { > - u16 status; > + pos = pci_find_capability(data.bridge, PCI_CAP_ID_PCIX); > + if (pos) { > + u16 status; If the leaf device is on the root bus, data.bridge will point to the leaf device itself (a Type 0 device) after pci_for_each_dma_alias() completes. Does this code incorrectly assume data.bridge is always a Type 1 bridge? If it evaluates a Type 0 device here, reading PCI_X_BRIDGE_SSTATUS aliases the Type 0 Command register. This could cause the code to read standard Type 1 PCI_SECONDARY_BUS and PCI_SUBORDINATE_BUS offsets (which fall into the middle of BAR2 for a Type 0 device), sending arbitrary BAR values as shadow bus ranges to the hypervisor. [ ... ] > @@ -249,10 +253,8 @@ static void hv_irq_compose_msi_msg(struct irq_data > *data, struct msi_msg *msg) > } > > stored_entry = kzalloc_obj(*stored_entry, GFP_ATOMIC); > - if (!stored_entry) { > - pr_debug("%s: failed to allocate chip data\n", __func__); > + if (!stored_entry) > return; > - } If kzalloc_obj() or hv_map_msi_interrupt() fails, this returns early without populating the struct msi_msg *msg parameter. Since callers (like msi_domain_activate() or irq_chip_compose_msi_msg()) allocate msg on the stack and cannot check for errors due to the void return type, does this result in uninitialized stack memory being blindly written to the hardware MSI registers? [ ... ] > @@ -344,13 +351,15 @@ static int hv_msi_domain_alloc(struct irq_domain *d, > unsigned int virq, unsigned > return ret; > > for (int i = 0; i < nr_irqs; ++i) { > - irq_domain_set_info(d, virq + i, 0, &hv_pci_msi_controller, > NULL, > - handle_edge_irq, NULL, "edge"); > + irq_domain_set_info(d, virq + i, 0, &hv_pci_msi_controller, > + NULL, handle_edge_irq, NULL, "edge"); > } > + > return 0; > } > > -static void hv_msi_domain_free(struct irq_domain *d, unsigned int virq, > unsigned int nr_irqs) > +static void hv_msi_domain_free(struct irq_domain *d, unsigned int virq, > + unsigned int nr_irqs) > { > for (int i = 0; i < nr_irqs; ++i) { > struct irq_data *irqd = irq_domain_get_irq_data(d, virq); Does this loop leak chip_data and leave IRQs mapped in the hypervisor for the remaining nr_irqs - 1 IRQs? It appears to call irq_domain_get_irq_data() using the base virq on every iteration instead of virq + i, causing hv_teardown_msi_irq() to be repeatedly invoked on the first IRQ only. > @@ -362,6 +371,7 @@ static void hv_msi_domain_free(struct irq_domain *d, > unsigned int virq, unsigned > > hv_teardown_msi_irq(to_pci_dev(desc->dev), irqd); > } > + > irq_domain_free_irqs_top(d, virq, nr_irqs); > } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
