netdev - Re: [PATCH net] net/sched: act_connmark: handle errno on tcf_idr_check

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:   Fri, 24 Feb 2023 10:05:11 +0100
From:   Simon Horman <simon.horman@...igine.com>
To:     Pedro Tammela <pctammela@...atatu.com>
Cc:     netdev@...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, error27@...il.com
Subject: Re: [PATCH net] net/sched: act_connmark: handle errno on
 tcf_idr_check_alloc

On Thu, Feb 23, 2023 at 11:16:39AM -0300, Pedro Tammela wrote:
> Smatch reports that 'ci' can be used uninitialized.
> The current code ignores errno coming from tcf_idr_check_alloc, which
> will lead to the incorrect usage of 'ci'. Handle the errno as it should.
> 
> Fixes: 288864effe33 ("net/sched: act_connmark: transition to percpu stats and rcu")
> Signed-off-by: Pedro Tammela <pctammela@...atatu.com>
> ---
>  net/sched/act_connmark.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/net/sched/act_connmark.c b/net/sched/act_connmark.c
> index 8dabfb52ea3d..cf4086a9e3c0 100644
> --- a/net/sched/act_connmark.c
> +++ b/net/sched/act_connmark.c
> @@ -125,6 +125,7 @@ static int tcf_connmark_init(struct net *net, struct nlattr *nla,
>  	if (!nparms)
>  		return -ENOMEM;
>  
> +	ci = to_connmark(*a);
>  	parm = nla_data(tb[TCA_CONNMARK_PARMS]);
>  	index = parm->index;
>  	ret = tcf_idr_check_alloc(tn, &index, a, bind);
> @@ -137,14 +138,11 @@ static int tcf_connmark_init(struct net *net, struct nlattr *nla,
>  			goto out_free;
>  		}
>  
> -		ci = to_connmark(*a);
> -
>  		nparms->net = net;
>  		nparms->zone = parm->zone;
>  
>  		ret = ACT_P_CREATED;
>  	} else if (ret > 0) {
> -		ci = to_connmark(*a);
>  		if (bind) {
>  			err = 0;
>  			goto out_free;

Hi Pedro,

I think the issue here isn't so much that there may be incorrect usage of
ci - although that can happen - but rather that an error condition - the
failure of tcf_idr_check_alloc is ignored.

Viewed through this lens I think it becomes clear that the hunk
below fixes the problem. While the hunks above are cleanups.
A nice cleanup. But still a cleanup.

I think that as a fix for 'net' a minimal approach is best and thus
the patch below.

I'd also like to comment that the usual style for kernel code is to handle
error cases in conditions - typically immediately after the condition
arises. While non-error cases follow, outside of condtions.

F.e.

	err = do_something(with_something);
	if (err) {
		/* handle error */
		...
	}

	/* proceed with non-error case here */
	...

In the code at hand this is complicate by there being two non-error cases,
and it thus being logical to treat them conditionally.

Even so, i do wonder if there is value in treating the error case first,
right next to the code that might cause the error, in order to make it
clearer that the error is being handled (as normal).

And in saying so, I do realise it contradicts my statement
about minimal changes to some extent.

i.e. (*completely untested*)

	ret = tcf_idr_check_alloc(tn, &index, a, bind);
	if (ret < 0) {
		err = ret;
		goto out_free;
	} else if (!ret) {
		...
	} else {
		...
	}

> @@ -158,6 +156,9 @@ static int tcf_connmark_init(struct net *net, struct nlattr *nla,
>  		nparms->zone = parm->zone;
>  
>  		ret = 0;
> +	} else {
> +		err = ret;
> +		goto out_free;
>  	}
>  
>  	err = tcf_action_check_ctrlact(parm->action, tp, &goto_ch, extack);
> -- 
> 2.34.1
>