| Message ID | 20211106204544.13136-1-phil@nwl.cc |
|---|---|
| State | Accepted |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | [iptables] Unbreak xtables-translate | expand |
On 2021-11-06, at 21:45:44 +0100, Phil Sutter wrote: > Fixed commit broke xtables-translate which still relied upon > do_parse() to properly initialize the passed iptables_command_state > reference. To allow for callers to preset fields, this doesn't happen > anymore so do_command_xlate() has to initialize itself. Otherwise > garbage from stack is read leading to segfaults and program aborts. > > Although init_cs callback is used by arptables only and > arptables-translate has not been implemented, do call it if set just > to avoid future issues. > > Fixes: cfdda18044d81 ("nft-shared: Introduce init_cs family ops callback") > Signed-off-by: Phil Sutter <phil@nwl.cc> > --- > iptables/xtables-translate.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/iptables/xtables-translate.c b/iptables/xtables-translate.c > index 086b85d2f9cef..e2948c5009dd6 100644 > --- a/iptables/xtables-translate.c > +++ b/iptables/xtables-translate.c > @@ -253,11 +253,18 @@ static int do_command_xlate(struct nft_handle *h, int argc, char *argv[], > .restore = restore, > .xlate = true, > }; > - struct iptables_command_state cs; > + struct iptables_command_state cs = { > + .jumpto = "", > + .argv = argv, > + }; No need to initialize .jumpto explicitly: initializing .argv will zero-initialize all the other members. > + > struct xtables_args args = { > .family = h->family, > }; > > + if (h->ops->init_cs) > + h->ops->init_cs(&cs); > + > do_parse(h, argc, argv, &p, &cs, &args); > > cs.restore = restore; > -- > 2.33.0 > >
On 2021-11-07, at 16:03:38 +0000, Jeremy Sowden wrote: > On 2021-11-06, at 21:45:44 +0100, Phil Sutter wrote: > > Fixed commit broke xtables-translate which still relied upon > > do_parse() to properly initialize the passed iptables_command_state > > reference. To allow for callers to preset fields, this doesn't > > happen anymore so do_command_xlate() has to initialize itself. > > Otherwise garbage from stack is read leading to segfaults and > > program aborts. > > > > Although init_cs callback is used by arptables only and > > arptables-translate has not been implemented, do call it if set just > > to avoid future issues. > > > > Fixes: cfdda18044d81 ("nft-shared: Introduce init_cs family ops callback") > > Signed-off-by: Phil Sutter <phil@nwl.cc> > > --- > > iptables/xtables-translate.c | 9 ++++++++- > > 1 file changed, 8 insertions(+), 1 deletion(-) > > > > diff --git a/iptables/xtables-translate.c b/iptables/xtables-translate.c > > index 086b85d2f9cef..e2948c5009dd6 100644 > > --- a/iptables/xtables-translate.c > > +++ b/iptables/xtables-translate.c > > @@ -253,11 +253,18 @@ static int do_command_xlate(struct nft_handle *h, int argc, char *argv[], > > .restore = restore, > > .xlate = true, > > }; > > - struct iptables_command_state cs; > > + struct iptables_command_state cs = { > > + .jumpto = "", > > + .argv = argv, > > + }; > > No need to initialize .jumpto explicitly: initializing .argv will > zero-initialize all the other members. Apologies, I'm talking nonsense: .jumpto is a pointer, not an array. Ignore me. :) > > + > > struct xtables_args args = { > > .family = h->family, > > }; > > > > + if (h->ops->init_cs) > > + h->ops->init_cs(&cs); > > + > > do_parse(h, argc, argv, &p, &cs, &args); > > > > cs.restore = restore; > > -- > > 2.33.0 > > > >
On Sat, Nov 06, 2021 at 09:45:44PM +0100, Phil Sutter wrote: > Fixed commit broke xtables-translate which still relied upon do_parse() > to properly initialize the passed iptables_command_state reference. To > allow for callers to preset fields, this doesn't happen anymore so > do_command_xlate() has to initialize itself. Otherwise garbage from > stack is read leading to segfaults and program aborts. > > Although init_cs callback is used by arptables only and > arptables-translate has not been implemented, do call it if set just to > avoid future issues. > > Fixes: cfdda18044d81 ("nft-shared: Introduce init_cs family ops callback") > Signed-off-by: Phil Sutter <phil@nwl.cc> Tested-by: Pablo Neira Ayuso <pablo@netfilter.org>
Hey Jeremy, On Sun, Nov 07, 2021 at 04:07:27PM +0000, Jeremy Sowden wrote: [...] > Apologies, I'm talking nonsense: .jumpto is a pointer, not an array. > Ignore me. :) I wondered at first, but indeed assigning an empty string to an array is identical to setting all fields zero. :) Thanks for the review! Cheers, Phil
diff --git a/iptables/xtables-translate.c b/iptables/xtables-translate.c index 086b85d2f9cef..e2948c5009dd6 100644 --- a/iptables/xtables-translate.c +++ b/iptables/xtables-translate.c @@ -253,11 +253,18 @@ static int do_command_xlate(struct nft_handle *h, int argc, char *argv[], .restore = restore, .xlate = true, }; - struct iptables_command_state cs; + struct iptables_command_state cs = { + .jumpto = "", + .argv = argv, + }; + struct xtables_args args = { .family = h->family, }; + if (h->ops->init_cs) + h->ops->init_cs(&cs); + do_parse(h, argc, argv, &p, &cs, &args); cs.restore = restore;
Fixed commit broke xtables-translate which still relied upon do_parse() to properly initialize the passed iptables_command_state reference. To allow for callers to preset fields, this doesn't happen anymore so do_command_xlate() has to initialize itself. Otherwise garbage from stack is read leading to segfaults and program aborts. Although init_cs callback is used by arptables only and arptables-translate has not been implemented, do call it if set just to avoid future issues. Fixes: cfdda18044d81 ("nft-shared: Introduce init_cs family ops callback") Signed-off-by: Phil Sutter <phil@nwl.cc> --- iptables/xtables-translate.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)