Re: [PATCH] net: 3com: 3c59x: add a check against null pointer dereference

From: Maciej W. Rozycki
Date: Tue Jul 20 2021 - 13:33:04 EST


On Tue, 15 Jun 2021, Zheyu Ma wrote:

> > > When the driver is processing the interrupt, it will read the value of
> > > the register to determine the status of the device. If the device is in
> > > an incorrect state, the driver may mistakenly enter this branch. At this
> > > time, the dma buffer has not been allocated, which will result in a null
> > > pointer dereference.
[...]
> > > diff --git a/drivers/net/ethernet/3com/3c59x.c b/drivers/net/ethernet/3com/3c59x.c
> > > index 741c67e546d4..e27901ded7a0 100644
> > > --- a/drivers/net/ethernet/3com/3c59x.c
> > > +++ b/drivers/net/ethernet/3com/3c59x.c
> > > @@ -2300,7 +2300,7 @@ _vortex_interrupt(int irq, struct net_device *dev)
> > > }
> > >
> > > if (status & DMADone) {
> > > - if (ioread16(ioaddr + Wn7_MasterStatus) & 0x1000) {
> > > + if ((ioread16(ioaddr + Wn7_MasterStatus) & 0x1000) && vp->tx_skb_dma) {
> > > iowrite16(0x1000, ioaddr + Wn7_MasterStatus); /* Ack the event. */
> > > dma_unmap_single(vp->gendev, vp->tx_skb_dma, (vp->tx_skb->len + 3) & ~3, DMA_TO_DEVICE);
> > > pkts_compl++;
> >
> > This means you won't be ack'ing the event - is this unacknowledged event
> > going to cause an issue later?
> >
>
> First, I'm not an expert in networking, but from my perspective, I
> don't think this will cause a problem. Because when the driver enters
> this branch, It means that it thinks that the hardware has already
> performed a DMA operation, and the driver only needs to do some
> follow-up work, but this is not the case. At this time,
> 'vp->tx_skb_dma' is still a null pointer, so there is no need for
> follow-up work at this time, it is meaningless, and it is appropriate
> not to perform any operations at this time.

What are the circumstances you observe this behaviour under? The state
of hardware is supposed to be consistent with the state of the driver. If
an inconsistency happens, then there are various possible causes such as:

1. The driver has a bug (in which case you need to track the bug down and
fix it).

2. The hardware does not behave as specified, e.g. due to an erratum (in
which case you need to track the problem down and work it around in the
driver).

3. The hardware may have been disturbed, e.g. due to EMC interference (in
which case you may implement a recovery attempt by reinitialising the
hardware once an odd state has been discovered).

4. The hardware is broken (throw it away).

For #4 the solution is obvious. For #3 you might want to implement a
hardware reset path rather than ignoring the inconsistent state and only
prevent the driver from crashing. If you have a way to reproduce the
issue, which I gather you do, then it's likely not #3 as that would be
intermittent, and then you'll have to investigate what is causing the
problem to see if it is #1 or #2 (or maybe #4), and act accordingly.

Someone more familiar with this hardware (is there a spec available?)
might be able to assist you once you have figured out what the exact
scenario leading to the failure you have observed is.

HTH,

Maciej