linux-kernel - Re: [PATCH v1 net-next 2/4] tc: flower: support for SPI

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 3 Aug 2023 14:55:46 +0300
From:   Dan Carpenter <dan.carpenter@...aro.org>
To:     Simon Horman <horms@...nel.org>
Cc:     Ratheesh Kannoth <rkannoth@...vell.com>, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org, sgoutham@...vell.com,
        lcherian@...vell.com, gakula@...vell.com, jerinj@...vell.com,
        hkelam@...vell.com, sbhatta@...vell.com, davem@...emloft.net,
        edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com,
        jhs@...atatu.com, xiyou.wangcong@...il.com, jiri@...nulli.us
Subject: Re: [PATCH v1 net-next 2/4] tc: flower: support for SPI

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