[-stable] igbvf: integer wrapping bug setting the mtu

If new_mtu is very large then "new_mtu + ETH_HLEN + ETH_FCS_LEN" can
wrap and the check on the next line can underflow. This is one of those
bugs which can be triggered by the user if you have namespaces
configured.

This is a static checker fix and I'm not sure what the impact is.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

> If new_mtu is very large then "new_mtu + ETH_HLEN + ETH_FCS_LEN" can
> wrap and the check on the next line can underflow. This is one of those
> bugs which can be triggered by the user if you have namespaces
> configured.
> 
> This is a static checker fix and I'm not sure what the impact is.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> diff --git a/drivers/net/ethernet/intel/igbvf/netdev.c b/drivers/net/ethernet/intel/igbvf/netdev.c
> index 95d5430..24e3883 100644
> --- a/drivers/net/ethernet/intel/igbvf/netdev.c
> +++ b/drivers/net/ethernet/intel/igbvf/netdev.c
> @@ -2342,7 +2342,7 @@ static struct net_device_stats *igbvf_get_stats(struct net_device *netdev)
>  static int igbvf_change_mtu(struct net_device *netdev, int new_mtu)
>  {
>  	struct igbvf_adapter *adapter = netdev_priv(netdev);
> -	int max_frame = new_mtu + ETH_HLEN + ETH_FCS_LEN;
> +	unsigned int max_frame = new_mtu + ETH_HLEN + ETH_FCS_LEN;
> 
>  	if ((new_mtu < 68) || (max_frame > MAX_JUMBO_FRAME_SIZE)) {
>  		dev_err(&adapter->pdev->dev, "Invalid MTU setting\n");

It is safer to check:
  	if ((new_mtu < 68) || (new_mtu > MAX_JUMBO_FRAME_SIZE - ETH_HLEN - ETH_FCS_LEN)) {

	David



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Fri, Sep 13, 2013 at 09:58:25AM +0100, David Laight wrote:
> > If new_mtu is very large then "new_mtu + ETH_HLEN + ETH_FCS_LEN" can
> > wrap and the check on the next line can underflow. This is one of those
> > bugs which can be triggered by the user if you have namespaces
> > configured.
> > 
> > This is a static checker fix and I'm not sure what the impact is.
> > 
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > 
> > diff --git a/drivers/net/ethernet/intel/igbvf/netdev.c b/drivers/net/ethernet/intel/igbvf/netdev.c
> > index 95d5430..24e3883 100644
> > --- a/drivers/net/ethernet/intel/igbvf/netdev.c
> > +++ b/drivers/net/ethernet/intel/igbvf/netdev.c
> > @@ -2342,7 +2342,7 @@ static struct net_device_stats *igbvf_get_stats(struct net_device *netdev)
> >  static int igbvf_change_mtu(struct net_device *netdev, int new_mtu)
> >  {
> >  	struct igbvf_adapter *adapter = netdev_priv(netdev);
> > -	int max_frame = new_mtu + ETH_HLEN + ETH_FCS_LEN;
> > +	unsigned int max_frame = new_mtu + ETH_HLEN + ETH_FCS_LEN;
> > 
> >  	if ((new_mtu < 68) || (max_frame > MAX_JUMBO_FRAME_SIZE)) {
> >  		dev_err(&adapter->pdev->dev, "Invalid MTU setting\n");
> 
> It is safer to check:
>   	if ((new_mtu < 68) || (new_mtu > MAX_JUMBO_FRAME_SIZE - ETH_HLEN - ETH_FCS_LEN)) {
> 

I believe my fix is already 100% safe...  Where is the bug in my code?

Your fix harder to read because of the additional math and because it's
checking "new_mtu" when we care about "max_frame".

regards,
dan carpenter

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

> > >  static int igbvf_change_mtu(struct net_device *netdev, int new_mtu)
> > >  {
> > >  	struct igbvf_adapter *adapter = netdev_priv(netdev);
> > > -	int max_frame = new_mtu + ETH_HLEN + ETH_FCS_LEN;
> > > +	unsigned int max_frame = new_mtu + ETH_HLEN + ETH_FCS_LEN;
> > >
> > >  	if ((new_mtu < 68) || (max_frame > MAX_JUMBO_FRAME_SIZE)) {
> > >  		dev_err(&adapter->pdev->dev, "Invalid MTU setting\n");
> >
> > It is safer to check:
> >   	if ((new_mtu < 68) || (new_mtu > MAX_JUMBO_FRAME_SIZE - ETH_HLEN - ETH_FCS_LEN)) {
> >
> 
> I believe my fix is already 100% safe...  Where is the bug in my code?
> 
> Your fix harder to read because of the additional math and because it's
> checking "new_mtu" when we care about "max_frame".

I don't like the window check on two variables.

However if ETH_HLEN and ETH_FCS_LEN are 'int' (not an unsigned type)
and 'new_mtu' is just below MAX_INT then the signed arithmetic
could overflow - generating an indeterminate value.
In which case 'max_frame' might not exceed MAX_JUMBO_FRAME_SIZE.

	David



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html