Uninitialized next pointer for pci device may crash kernel

[rick-masters]

When a pci device structure is added to the new linked list implementation its next pointer may be uninitialized. It should be set to NULL.

A new struct pci_device is initialized from a passed in variable pci_dev: https://github.com/mikaku/Fiwix/blob/d1a53abd1453f8f2aeb092a06077d1b40ed35e86/drivers/pci/pci.c#L99

However, that structure is allocated on the stack in scan_bus so some members may be uninitialized: https://github.com/mikaku/Fiwix/blob/d1a53abd1453f8f2aeb092a06077d1b40ed35e86/drivers/pci/pci.c#L112-L117

If pci_dev has a non-NULL next pointer, the kernel may attempt to access that memory which may be invalid and may crash the kernel.

Whether the crash is reproducible depends on the contents of stack memory which depends on many factors, so I have no easy way of providing a test case that triggers the problem. It always crashes for me and setting next to NULL fixes it.

[mikaku]

Good point.

I think that this line:

https://github.com/mikaku/Fiwix/blob/d1a53abd1453f8f2aeb092a06077d1b40ed35e86/drivers/pci/pci.c#L91

is useless because pdt is clobbered by the contents of pci_dev some lines below.

I think that instead of setting pdt->next = NULL; we could remove the line 91 and put a similar line inside the scan_bus() loop. So, we make sure that pci_dev is properly initialized from the beginning.

What do you think?

[mikaku]

BTW, It's amazing how you are viewing this code at the same time I was. Today I've been precisely touching these PCI functions. :smiley:

[rick-masters]

Yes, initializing pci_dev sounds like a better solution.

[mikaku]

I think there are still other bugs because pci_get_device() enters in an infinite loop when it doesn't found the vendor and device ids.

[rick-masters]

Hmm, looks to me like it should exit the while loop when it reaches the end of the list.

[mikaku]

Thank you.