Re: [PATCH v5] net: caif: fix stack out-of-bounds write in cfctrl_link_setup()

[Kangzheng Gu]

Thanks for all of your advice, I am preparing a new version of patch now.

Simon Horman <horms@xxxxxxxxxx> 于2026年4月14日周二 19:29写道：
>
> On Mon, Apr 13, 2026 at 11:30:53AM +0200, Paolo Abeni wrote:
> > On 4/12/26 3:57 PM, Simon Horman wrote:
> > > I am wondering if it would be best to follow the pattern for
> > > writing linkparam.u.utility.name elsewhere in this function.
> > > That:
> > > 1. Uses a somewhat more succinct loop control structure
> > > 2. Silently truncates input without updating cmdrsp if overrun would occur
> > >
> > > Something like this (compile tested only!):
> > >
> > > diff --git a/net/caif/cfctrl.c b/net/caif/cfctrl.c
> > > index c6cc2bfed65d..ba184c11386e 100644
> > > --- a/net/caif/cfctrl.c
> > > +++ b/net/caif/cfctrl.c
> > > @@ -15,6 +15,7 @@
> > >  #include <net/caif/cfctrl.h>
> > >
> > >  #define container_obj(layr) container_of(layr, struct cfctrl, serv.layer)
> > > +#define RFM_VOLUME_LEN 20
> > >  #define UTILITY_NAME_LENGTH 16
> > >  #define CFPKT_CTRL_PKT_LEN 20
> > >
> > > @@ -414,10 +415,11 @@ static int cfctrl_link_setup(struct cfctrl *cfctrl, struct cfpkt *pkt, u8 cmdrsp
> > >              */
> > >             linkparam.u.rfm.connid = cfpkt_extr_head_u32(pkt);
> > >             cp = (u8 *) linkparam.u.rfm.volume;
> > > -           for (tmp = cfpkt_extr_head_u8(pkt);
> > > -                cfpkt_more(pkt) && tmp != '\0';
> > > -                tmp = cfpkt_extr_head_u8(pkt))
> > > +           caif_assert(sizeof(linkparam.u.rfm.volume) >= RFM_VOLUME_LEN);
> > > +           for(i = 0; i < RFM_VOLUME_LEN - 1 && cfpkt_more(pkt); i++) {
> > > +                   tmp = cfpkt_extr_head_u8(pkt);
> > >                     *cp++ = tmp;
> > > +           }
> > >             *cp = '\0';
> > >
> > >             if (CFCTRL_ERR_BIT & cmdrsp)
> >
> > I agree that the code suggested by Simon is clearer. Note that AFAICS it
> > lacks an additional `tmp!= '\0'` check to break the loop, but even with
> > that added it should be preferable.
>
> Sorry, I left out the `tmp!= '\0' check.
> That was unintentional and I agree it should be there.