Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 3 Aug 2023 14:55:46 +0300
From:   Dan Carpenter <dan.carpenter@linaro.org>
To:     Simon Horman <horms@kernel.org>
Cc:     Ratheesh Kannoth <rkannoth@marvell.com>, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org, sgoutham@marvell.com,
        lcherian@marvell.com, gakula@marvell.com, jerinj@marvell.com,
        hkelam@marvell.com, sbhatta@marvell.com, davem@davemloft.net,
        edumazet@google.com, kuba@kernel.org, pabeni@redhat.com,
        jhs@mojatatu.com, xiyou.wangcong@gmail.com, jiri@resnulli.us
Subject: Re: [PATCH v1 net-next 2/4] tc: flower: support for SPI
Message-ID: <664b202a-d126-4708-a2af-94f768fe3abd@kadam.mountain>
References: <20230801014101.2955887-1-rkannoth@marvell.com>
 <20230801014101.2955887-3-rkannoth@marvell.com>
 <ZMqpd2DyHz4O/v17@kernel.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZMqpd2DyHz4O/v17@kernel.org>
Precedence: bulk

On Wed, Aug 02, 2023 at 09:07:35PM +0200, Simon Horman wrote:
> + Dan Carpenter
> 
> On Tue, Aug 01, 2023 at 07:10:59AM +0530, Ratheesh Kannoth wrote:
> > @@ -1894,6 +1915,12 @@ static int fl_set_key(struct net *net, struct nlattr **tb,
> >  			return ret;
> >  	}
> >  
> > +	if (tb[TCA_FLOWER_KEY_SPI]) {
> > +		ret = fl_set_key_spi(tb, key, mask, extack);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> 
> Hi Dan,
> 
> I'm seeing a warning from Smatch, which I think is a false positive,
> but I feel that I should raise. Perhaps you could take a look at it?
> 
> net/sched/cls_flower.c:1918 fl_set_key() error: buffer overflow 'tb' 106 <= 108
> 

You're using the cross function database, right?  What happens is that
when someone adds a new type of net link attribute, it takes a rebuild
for the database to sync up.

I can't think of a good way to fix this.  This information is passed as
a BUF_SIZE.  Each database rebuild passes the BUF_SIZE one call further
down the call tree.

$ smdb fl_set_key | grep BUF_SIZE
net/sched/cls_flower.c |            fl_change |           fl_set_key |           BUF_SIZE |  1 |              tb | 864
net/sched/cls_flower.c |      fl_tmplt_create |           fl_set_key |           BUF_SIZE |  1 |              tb | 864

This is a flaw in how Smatch works, and theoretically it affects
everything, but in practical terms it affect netlink attribute tables
the most.  Other places are not modified as often or they pass the size
as a parameter.  I could modify check_index_overflow.c to silence
warnings where it's a netlink attribute table and the offset is less
than __TCA_FLOWER_MAX.

regards,
dan carpenter