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

On Tue, 15 Jun 2021, Zheyu Ma wrote:

quoted

quoted

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.

[...]

quoted

quoted

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