| Message ID | 20230615-igc-err-ptr-v1-1-a17145eb8d62@kernel.org (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [RFC,net] igc: Avoid dereference of ptr_err in igc_clean_rx_irq() | expand |
On Thu, Jun 15, 2023 at 11:45:36AM +0200, Simon Horman wrote: Hi Simon, > In igc_clean_rx_irq() the result of a call to igc_xdp_run_prog() is assigned > to the skb local variable. This may be an ERR_PTR. > > A little later the following is executed, which seems to be a > possible dereference of an ERR_PTR. > > total_bytes += skb->len; > > Avoid this problem by continuing the loop in which all of the > above occurs once the handling of the NULL case completes. > > This proposed fix is speculative - I do not have deep knowledge of this > driver. And I am concerned about the effect of skipping the following > logic: > > igc_put_rx_buffer(rx_ring, rx_buffer, rx_buffer_pgcnt); > cleaned_count++; this will break - you have to recycle the buffer to have it going. > > Flagged by Smatch as: > > .../igc_main.c:2467 igc_xdp_run_prog() warn: passing zero to 'ERR_PTR' how about PTR_ERR_OR_ZERO() ? this would silence smatch and is not an intrusive change. another way is to get rid of ERR_PTR() around skb/xdp run result but i think the former would be just fine. > > Compile tested only. > > Fixes: 26575105d6ed ("igc: Add initial XDP support") > Signed-off-by: Simon Horman <horms@kernel.org> > --- > drivers/net/ethernet/intel/igc/igc_main.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c > index 88145c30c919..b58c8a674bd1 100644 > --- a/drivers/net/ethernet/intel/igc/igc_main.c > +++ b/drivers/net/ethernet/intel/igc/igc_main.c > @@ -2586,6 +2586,7 @@ static int igc_clean_rx_irq(struct igc_q_vector *q_vector, const int budget) > > total_packets++; > total_bytes += size; > + continue; > } else if (skb) > igc_add_rx_frag(rx_ring, rx_buffer, skb, size); > else if (ring_uses_build_skb(rx_ring)) > >
On Thu, Jun 15, 2023 at 11:55:29AM +0200, Maciej Fijalkowski wrote: > On Thu, Jun 15, 2023 at 11:45:36AM +0200, Simon Horman wrote: Hi Marciej, > Hi Simon, > > > In igc_clean_rx_irq() the result of a call to igc_xdp_run_prog() is assigned > > to the skb local variable. This may be an ERR_PTR. > > > > A little later the following is executed, which seems to be a > > possible dereference of an ERR_PTR. > > > > total_bytes += skb->len; > > > > Avoid this problem by continuing the loop in which all of the > > above occurs once the handling of the NULL case completes. > > > > This proposed fix is speculative - I do not have deep knowledge of this > > driver. And I am concerned about the effect of skipping the following > > logic: > > > > igc_put_rx_buffer(rx_ring, rx_buffer, rx_buffer_pgcnt); > > cleaned_count++; > > this will break - you have to recycle the buffer to have it going. Thanks. As I said I wasn't sure about the fix: it was a strawman. > > Flagged by Smatch as: > > > > .../igc_main.c:2467 igc_xdp_run_prog() warn: passing zero to 'ERR_PTR' > > how about PTR_ERR_OR_ZERO() ? this would silence smatch and is not an > intrusive change. another way is to get rid of ERR_PTR() around skb/xdp > run result but i think the former would be just fine. Sorry, there were two warnings. And I accidently trimmed the one that is more relevant instead of the one that is less relevant. I do agree the one above does not appear to be a bug. But I am concerned abut this one: .../igc_main.c:2618 igc_clean_rx_irq() error: 'skb' dereferencing possible ERR_PTR() If skb is an error pointer, e.g. ERR_PTR(-IGC_XDP_PASS), and it is dereferenced, that would be a problem, right? Perhaps I'm missing something obvious and this can't occur. But it does seem possible to me. > > > > > Compile tested only. > > > > Fixes: 26575105d6ed ("igc: Add initial XDP support") > > Signed-off-by: Simon Horman <horms@kernel.org> > > --- > > drivers/net/ethernet/intel/igc/igc_main.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c > > index 88145c30c919..b58c8a674bd1 100644 > > --- a/drivers/net/ethernet/intel/igc/igc_main.c > > +++ b/drivers/net/ethernet/intel/igc/igc_main.c > > @@ -2586,6 +2586,7 @@ static int igc_clean_rx_irq(struct igc_q_vector *q_vector, const int budget) > > > > total_packets++; > > total_bytes += size; > > + continue; > > } else if (skb) > > igc_add_rx_frag(rx_ring, rx_buffer, skb, size); > > else if (ring_uses_build_skb(rx_ring)) > > > > >
On Thu, Jun 15, 2023 at 01:27:47PM +0200, Simon Horman wrote: > On Thu, Jun 15, 2023 at 11:55:29AM +0200, Maciej Fijalkowski wrote: > > On Thu, Jun 15, 2023 at 11:45:36AM +0200, Simon Horman wrote: > > Hi Marciej, > > > Hi Simon, > > > > > In igc_clean_rx_irq() the result of a call to igc_xdp_run_prog() is assigned > > > to the skb local variable. This may be an ERR_PTR. > > > > > > A little later the following is executed, which seems to be a > > > possible dereference of an ERR_PTR. > > > > > > total_bytes += skb->len; > > > > > > Avoid this problem by continuing the loop in which all of the > > > above occurs once the handling of the NULL case completes. > > > > > > This proposed fix is speculative - I do not have deep knowledge of this > > > driver. And I am concerned about the effect of skipping the following > > > logic: > > > > > > igc_put_rx_buffer(rx_ring, rx_buffer, rx_buffer_pgcnt); > > > cleaned_count++; > > > > this will break - you have to recycle the buffer to have it going. > > Thanks. As I said I wasn't sure about the fix: it was a strawman. > > > > Flagged by Smatch as: > > > > > > .../igc_main.c:2467 igc_xdp_run_prog() warn: passing zero to 'ERR_PTR' > > > > how about PTR_ERR_OR_ZERO() ? this would silence smatch and is not an > > intrusive change. another way is to get rid of ERR_PTR() around skb/xdp > > run result but i think the former would be just fine. > > Sorry, there were two warnings. And I accidently trimmed the one > that is more relevant instead of the one that is less relevant. > I do agree the one above does not appear to be a bug. > > But I am concerned abut this one: > > .../igc_main.c:2618 igc_clean_rx_irq() error: 'skb' dereferencing possible ERR_PTR() > > If skb is an error pointer, e.g. ERR_PTR(-IGC_XDP_PASS), and > it is dereferenced, that would be a problem, right? IGC_XDP_PASS is 0. -0 is still 0 right? this means skb is NULL and igc_{build,construct}_skb() will init it. For ERR_PTR, igc_cleanup_headers() does IS_ERR() against it and continues. So you will get to line 2618 only for valid skb, it just happens that logic is written in a way that skb is supposed to carry XDP return code. We removed this in ice for example but i40e works like that for many years without issues, AFAICT... > > Perhaps I'm missing something obvious and this can't occur. > But it does seem possible to me. > > > > > > > > > Compile tested only. > > > > > > Fixes: 26575105d6ed ("igc: Add initial XDP support") > > > Signed-off-by: Simon Horman <horms@kernel.org> > > > --- > > > drivers/net/ethernet/intel/igc/igc_main.c | 1 + > > > 1 file changed, 1 insertion(+) > > > > > > diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c > > > index 88145c30c919..b58c8a674bd1 100644 > > > --- a/drivers/net/ethernet/intel/igc/igc_main.c > > > +++ b/drivers/net/ethernet/intel/igc/igc_main.c > > > @@ -2586,6 +2586,7 @@ static int igc_clean_rx_irq(struct igc_q_vector *q_vector, const int budget) > > > > > > total_packets++; > > > total_bytes += size; > > > + continue; > > > } else if (skb) > > > igc_add_rx_frag(rx_ring, rx_buffer, skb, size); > > > else if (ring_uses_build_skb(rx_ring)) > > > > > > > >
On Thu, Jun 15, 2023 at 02:09:45PM +0200, Maciej Fijalkowski wrote: > On Thu, Jun 15, 2023 at 01:27:47PM +0200, Simon Horman wrote: > > On Thu, Jun 15, 2023 at 11:55:29AM +0200, Maciej Fijalkowski wrote: > > > On Thu, Jun 15, 2023 at 11:45:36AM +0200, Simon Horman wrote: > > > > Hi Marciej, > > > > > Hi Simon, > > > > > > > In igc_clean_rx_irq() the result of a call to igc_xdp_run_prog() is assigned > > > > to the skb local variable. This may be an ERR_PTR. > > > > > > > > A little later the following is executed, which seems to be a > > > > possible dereference of an ERR_PTR. > > > > > > > > total_bytes += skb->len; > > > > > > > > Avoid this problem by continuing the loop in which all of the > > > > above occurs once the handling of the NULL case completes. > > > > > > > > This proposed fix is speculative - I do not have deep knowledge of this > > > > driver. And I am concerned about the effect of skipping the following > > > > logic: > > > > > > > > igc_put_rx_buffer(rx_ring, rx_buffer, rx_buffer_pgcnt); > > > > cleaned_count++; > > > > > > this will break - you have to recycle the buffer to have it going. > > > > Thanks. As I said I wasn't sure about the fix: it was a strawman. > > > > > > Flagged by Smatch as: > > > > > > > > .../igc_main.c:2467 igc_xdp_run_prog() warn: passing zero to 'ERR_PTR' > > > > > > how about PTR_ERR_OR_ZERO() ? this would silence smatch and is not an > > > intrusive change. another way is to get rid of ERR_PTR() around skb/xdp > > > run result but i think the former would be just fine. > > > > Sorry, there were two warnings. And I accidently trimmed the one > > that is more relevant instead of the one that is less relevant. > > I do agree the one above does not appear to be a bug. > > > > But I am concerned abut this one: > > > > .../igc_main.c:2618 igc_clean_rx_irq() error: 'skb' dereferencing possible ERR_PTR() > > > > If skb is an error pointer, e.g. ERR_PTR(-IGC_XDP_PASS), and > > it is dereferenced, that would be a problem, right? > > IGC_XDP_PASS is 0. -0 is still 0 right? Yes, I missed that point. Though I could have chosen a different value which is not zero. > this means skb is NULL and igc_{build,construct}_skb() will init it. For > ERR_PTR, igc_cleanup_headers() does IS_ERR() against it and continues. So > you will get to line 2618 only for valid skb, it just happens that logic > is written in a way that skb is supposed to carry XDP return code. We > removed this in ice for example but i40e works like that for many years > without issues, AFAICT... Thanks. I now see that the key point I was missing is the IS_ERR() check in igc_cleanup_headers(). I agree this is not a bug.
The original code is okay. Passing zero to ERR_PTR() is intentional. On Thu, Jun 15, 2023 at 11:45:36AM +0200, Simon Horman wrote: > In igc_clean_rx_irq() the result of a call to igc_xdp_run_prog() is assigned > to the skb local variable. This may be an ERR_PTR. > > A little later the following is executed, which seems to be a > possible dereference of an ERR_PTR. > > total_bytes += skb->len; There is an IS_ERR() check in igc_cleanup_headers() which prevents this. Sort of tricky to see. Do you have the cross function database set up? If so then Smatch shouldn't warn about this dereference. > > Avoid this problem by continuing the loop in which all of the > above occurs once the handling of the NULL case completes. > > This proposed fix is speculative - I do not have deep knowledge of this > driver. And I am concerned about the effect of skipping the following > logic: > > igc_put_rx_buffer(rx_ring, rx_buffer, rx_buffer_pgcnt); > cleaned_count++; > > Flagged by Smatch as: > > .../igc_main.c:2467 igc_xdp_run_prog() warn: passing zero to 'ERR_PTR' Linus once complained to me that this check is bogus and passing zero to ERR_PTR() is fine and an intended use case. But actually this test does really find a lot of bugs. I think for new warnings it is less than 10% false positives. But we fix the bugs so warnings which are over three month old are probably 97% false positives. regards, dan carpenter
diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c index 88145c30c919..b58c8a674bd1 100644 --- a/drivers/net/ethernet/intel/igc/igc_main.c +++ b/drivers/net/ethernet/intel/igc/igc_main.c @@ -2586,6 +2586,7 @@ static int igc_clean_rx_irq(struct igc_q_vector *q_vector, const int budget) total_packets++; total_bytes += size; + continue; } else if (skb) igc_add_rx_frag(rx_ring, rx_buffer, skb, size); else if (ring_uses_build_skb(rx_ring))
In igc_clean_rx_irq() the result of a call to igc_xdp_run_prog() is assigned to the skb local variable. This may be an ERR_PTR. A little later the following is executed, which seems to be a possible dereference of an ERR_PTR. total_bytes += skb->len; Avoid this problem by continuing the loop in which all of the above occurs once the handling of the NULL case completes. This proposed fix is speculative - I do not have deep knowledge of this driver. And I am concerned about the effect of skipping the following logic: igc_put_rx_buffer(rx_ring, rx_buffer, rx_buffer_pgcnt); cleaned_count++; Flagged by Smatch as: .../igc_main.c:2467 igc_xdp_run_prog() warn: passing zero to 'ERR_PTR' Compile tested only. Fixes: 26575105d6ed ("igc: Add initial XDP support") Signed-off-by: Simon Horman <horms@kernel.org> --- drivers/net/ethernet/intel/igc/igc_main.c | 1 + 1 file changed, 1 insertion(+)