netdev - Re: [PATCH net-next] flow_dissector: Add IPSEC dissectors

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] [day] [month] [year] [list]

Message-ID: <ZMDNjD46BvZ5zp5I@corigine.com>
Date: Wed, 26 Jul 2023 09:38:52 +0200
From: Simon Horman <simon.horman@...igine.com>
To: Ratheesh Kannoth <rkannoth@...vell.com>
Cc: netdev@...r.kernel.org, linux-kernel@...r.kernel.org, jhs@...atatu.com,
	xiyou.wangcong@...il.com, jiri@...nulli.us, davem@...emloft.net,
	edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com,
	Sunil Kovvuri Goutham <sgoutham@...vell.com>
Subject: Re: [PATCH net-next] flow_dissector: Add IPSEC dissectors

On Wed, Jul 26, 2023 at 06:34:34AM +0000, Ratheesh Kannoth wrote:
> > From: Simon Horman <simon.horman@...igine.com>
> > Sent: Wednesday, July 26, 2023 1:21 AM
> > Subject: [EXT] Re: [PATCH net-next] flow_dissector: Add IPSEC dissectors
> 
> 
> > >  	FLOW_DISSECTOR_KEY_NUM_OF_VLANS, /* struct
> > flow_dissector_key_num_of_vlans */
> > >  	FLOW_DISSECTOR_KEY_PPPOE, /* struct flow_dissector_key_pppoe
> > */
> > >  	FLOW_DISSECTOR_KEY_L2TPV3, /* struct flow_dissector_key_l2tpv3
> > */
> > > +	FLOW_DISSECTOR_KEY_IPSEC, /* struct flow_dissector_key_ipsec */
> > >  	FLOW_DISSECTOR_KEY_CFM, /* struct flow_dissector_key_cfm */
> > >
> > >  	FLOW_DISSECTOR_KEY_MAX,
> > 
> > ...
> > 
> > Hi Ratheesh,
> > 
> > With this change, this enum now has 33 values, excluding
> > FLOW_DISSECTOR_KEY_MAX.  I.e the range of values is from 0 to 32.
> > 
> > But dissector_uses_key() looks like this:
> > 
> > 
> > static inline bool dissector_uses_key(const struct flow_dissector
> > *flow_dissector,
> >                                       enum flow_dissector_key_id key_id) {
> >         return flow_dissector->used_keys & (1 << key_id); }
> > 
> > And the type of the used_keys field of struct flow_dissector is unsigned int, a
> > 32bit entity.
> > 
> > So an overflow will now occur if key_id is FLOW_DISSECTOR_KEY_CFM.
> > 
> > This is flagged by Sparse.
> > 

Hi Ratheesh,

> Thank you !
> 1)  How did you run sparse to detect this error. When I ran  below command, it did not throw this error/warning ?
>        make  C=2 net/core/ V=s

I ran:

	make C=2 net/core/flow_dissector.o

FWIIW, I also saw the warning using make  C=2 net/core/ V=s


>       sparse version is 0.6.4

I am using sparse git commit ce1a6720f69e, because at one point that
was the newest revision at a time the packaged version I had wasn't working
for me, and that was the latest commit at the time.

       https://git.kernel.org/pub/scm/devel/sparse/sparse.git/commit/ce1a6720f69e

> 2)   Is it okay to change  variable type of  "used_keys"  from "unsigned int" to "unsigned long long" to accommodate this.
>        This variable is used at lot of places in the code.

I might go for u64.
But yes, I suspect making the variable wider is a viable solution.

Given that it is widely used it would be good to inspect
how it is used to make sure this change makes sense.

I would make this change a separate patch if it is more than one line.

Also, while you are there, can you fix the spelling mistake in
the comment on the 'used_keys' line in struct flow_dissector ?
It's the same line that needs to be changed to change the type of 'used_keys'.