Re: [PATCH net v3] net: fec: Propagate PTP initialization errors

[Bui Duc Phuc]

> > -             if (irq < 0)
> > +             if (irq < 0 && irq != -ENXIO)
> > +                     return irq;
>
> [Severity: High]
> Does this direct return bypass the error handling cleanup at the end of
> fec_probe()?
>
> By returning directly instead of jumping to the failed_irq label, the driver
> bypasses fec_ptp_stop(). This leaves the fep->time_keep delayed work
> (scheduled earlier in fec_ptp_init()) active.
>
> It also bypasses free_netdev(ndev), which leaks the net_device structure.
>
> When the driver core cleans up devres allocations on probe failure, it unmaps
> fep->hwp. If the un-canceled delayed work subsequently fires, will it attempt
> to access the unmapped memory at fep->hwp and cause a kernel panic?
>


Yes, this was my mistake.

It should be:
+ ret = irq;
+ goto failed_irq;

instead of :
+ return irq;.

I will fix this in the next version.



> [Severity: High]
> Does this error return leave the devm-managed IRQ active while propagating an
> error back to the probe function?
>
> If ptp_clock_register() fails, fec_ptp_init() returns an error which
> propagates to fec_probe(), causing it to jump to failed_reset.
>
> The error path in fec_probe() manually frees ndev via free_netdev(ndev).
>
> However, the PPS interrupt was registered just prior using devm_request_irq(),
> so it remains active until fec_probe() returns and devres tears it down.
>
> If the PPS interrupt fires between the manual free of ndev and the devres
> cleanup, will fec_pps_interrupt() cast dev_id to ndev and attempt to
> dereference the already-freed ndev structure, causing a use-after-free crash?


This is a pre-existing issue:
https://lore.kernel.org/all/CAABR9nEkci1OFoHx2qmJWvKabUN=JSu5AcPynQ-tH02wxHEmOA@xxxxxxxxxxxxxx/

Previously, I suggested using request_irq() and free_irq() manually,
but that would add more code.
Another option is to replace alloc_etherdev_mqs() with
devm_alloc_etherdev_mqs(). With the LIFO cleanup order,
devm_request_irq() will be cleaned up before free_netdev(res->ndev),
which should resolve the issue.

Since this is a long-standing, pre-existing issue, I think it would be
better to handle it in a separate patch.