netdev - Re: [PATCH] net/sched: Forbid assigning mirred action to a filter attached to the egress

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, 14 Mar 2024 11:47:15 -0300
From: Pedro Tammela <pctammela@...atatu.com>
To: renmingshuai <renmingshuai@...wei.com>, jiri@...nulli.us
Cc: caowangbao@...wei.com, davem@...emloft.net, jhs@...atatu.com,
 liaichun@...wei.com, netdev@...r.kernel.org, vladbu@...dia.com,
 xiyou.wangcong@...il.com, yanan@...wei.com
Subject: Re: [PATCH] net/sched: Forbid assigning mirred action to a filter
 attached to the egress

On 14/03/2024 11:04, renmingshuai wrote:
>>> ---
>>> net/sched/act_mirred.c                        |  4 +++
>>> .../tc-testing/tc-tests/actions/mirred.json   | 32 +++++++++++++++++++
>>> 2 files changed, 36 insertions(+)
>>>
>>> diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
>>> index 5b3814365924..fc96705285fb 100644
>>> --- a/net/sched/act_mirred.c
>>> +++ b/net/sched/act_mirred.c
>>> @@ -120,6 +120,10 @@ static int tcf_mirred_init(struct net *net, struct nlattr *nla,
>>> 		NL_SET_ERR_MSG_MOD(extack, "Mirred requires attributes to be passed");
>>> 		return -EINVAL;
>>> 	}
>>> +	if (tp->chain->block->q->parent != TC_H_INGRESS) {
>>> +		NL_SET_ERR_MSG_MOD(extack, "Mirred can only be assigned to the filter attached to ingress");
>>
>> Hmm, that is quite restrictive. I'm pretty sure you would break some
>> valid usecases.
> Hmm, that is really quite restrictive. It might be better to Forbid mirred attached to egress filter
> to mirror or redirect packets to the egress.
> 
> diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
> index fc96705285fb..ab3841470992 100644
> --- a/net/sched/act_mirred.c
> +++ b/net/sched/act_mirred.c
> @@ -152,6 +152,11 @@ static int tcf_mirred_init(struct net *net, struct nlattr *nla,
>                  return -EINVAL;
>          }
> 
> +       if ((tp->chain->block->q->parent != TC_H_INGRESS) && (parm->eaction == TCA_EGRESS_MIRROR || parm->eaction == TCA_EGRESS_REDIR)) {
> +               NL_SET_ERR_MSG_MOD(extack, "Mirred assigned to egress filter can only mirror or redirect to ingress");
> +               return -EINVAL;
> +       }
> +
>          switch (parm->eaction) {
>          case TCA_EGRESS_MIRROR:
>          case TCA_EGRESS_REDIR:


Are you sure this happens to egress mirred to netdevs other than the one 
used to attach the filter?
It seems like in your example you are forcing mirred egress to the same 
netdev as the filter