1. Patchew
  2. linux
  3. View series

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

Kangzheng Gu posted 1 patch 2 months ago 
net/caif/cfctrl.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
[PATCH v5] net: caif: fix stack out-of-bounds write in cfctrl_link_setup()
Posted by Kangzheng Gu 2 months ago 
cfctrl_link_setup() copies the RFM volume name from a received control
packet into linkparam.u.rfm.volume until a '\0' is found. A malformed
packet can omit the terminator and make the copy run past the 20-byte
stack buffer.

Stop copying once the buffer is full and mark the frame as failed by
setting CFCTRL_ERR_BIT so the link setup is rejected.

Fixes: b482cd2053e3 ("net-caif: add CAIF core protocol stack")
Cc: stable@vger.kernel.org
Signed-off-by: Kangzheng Gu <xiaoguai0992@gmail.com>
---
 v5:
 - remove the Reported-by.
 - print a warn message and reject link setup by setting CFCTRL_ERR_BIT.
 - using %zu to adapt the compilation of 32-bit kernel.
 - add rate limit to error message

 net/caif/cfctrl.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/net/caif/cfctrl.c b/net/caif/cfctrl.c
index c6cc2bfed65d..373ab1dc67a7 100644
--- a/net/caif/cfctrl.c
+++ b/net/caif/cfctrl.c
@@ -416,8 +416,16 @@ static int cfctrl_link_setup(struct cfctrl *cfctrl, struct cfpkt *pkt, u8 cmdrsp
 		cp = (u8 *) linkparam.u.rfm.volume;
 		for (tmp = cfpkt_extr_head_u8(pkt);
 		     cfpkt_more(pkt) && tmp != '\0';
-		     tmp = cfpkt_extr_head_u8(pkt))
+		     tmp = cfpkt_extr_head_u8(pkt)) {
+			if (cp >= (u8 *)linkparam.u.rfm.volume +
+			    sizeof(linkparam.u.rfm.volume) - 1) {
+				pr_warn_ratelimited("Request reject, volume name length exceeds %zu\n",
+						    sizeof(linkparam.u.rfm.volume));
+				cmdrsp |= CFCTRL_ERR_BIT;
+				break;
+			}
 			*cp++ = tmp;
+		}
 		*cp = '\0';
 
 		if (CFCTRL_ERR_BIT & cmdrsp)
-- 
2.50.1
Re: [PATCH v5] net: caif: fix stack out-of-bounds write in cfctrl_link_setup()
Posted by Simon Horman 2 months ago 
On Wed, Apr 08, 2026 at 12:53:33PM +0000, Kangzheng Gu wrote:
> cfctrl_link_setup() copies the RFM volume name from a received control
> packet into linkparam.u.rfm.volume until a '\0' is found. A malformed
> packet can omit the terminator and make the copy run past the 20-byte
> stack buffer.
> 
> Stop copying once the buffer is full and mark the frame as failed by
> setting CFCTRL_ERR_BIT so the link setup is rejected.
> 
> Fixes: b482cd2053e3 ("net-caif: add CAIF core protocol stack")
> Cc: stable@vger.kernel.org
> Signed-off-by: Kangzheng Gu <xiaoguai0992@gmail.com>
> ---
>  v5:
>  - remove the Reported-by.
>  - print a warn message and reject link setup by setting CFCTRL_ERR_BIT.
>  - using %zu to adapt the compilation of 32-bit kernel.
>  - add rate limit to error message
> 
>  net/caif/cfctrl.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/net/caif/cfctrl.c b/net/caif/cfctrl.c
> index c6cc2bfed65d..373ab1dc67a7 100644
> --- a/net/caif/cfctrl.c
> +++ b/net/caif/cfctrl.c
> @@ -416,8 +416,16 @@ static int cfctrl_link_setup(struct cfctrl *cfctrl, struct cfpkt *pkt, u8 cmdrsp
>  		cp = (u8 *) linkparam.u.rfm.volume;
>  		for (tmp = cfpkt_extr_head_u8(pkt);
>  		     cfpkt_more(pkt) && tmp != '\0';
> -		     tmp = cfpkt_extr_head_u8(pkt))
> +		     tmp = cfpkt_extr_head_u8(pkt)) {
> +			if (cp >= (u8 *)linkparam.u.rfm.volume +
> +			    sizeof(linkparam.u.rfm.volume) - 1) {
> +				pr_warn_ratelimited("Request reject, volume name length exceeds %zu\n",
> +						    sizeof(linkparam.u.rfm.volume));
> +				cmdrsp |= CFCTRL_ERR_BIT;
> +				break;
> +			}
>  			*cp++ = tmp;
> +		}
>  		*cp = '\0';
>  
>  		if (CFCTRL_ERR_BIT & cmdrsp)

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)

Also, it seems that writing linkparam.u.utility.paramlen elsewhere
in this function also has a potential buffer overrun (by one byte).
Re: [PATCH v5] net: caif: fix stack out-of-bounds write in cfctrl_link_setup()
Posted by Paolo Abeni 2 months ago 
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.

Thanks,

Paolo
Re: [PATCH v5] net: caif: fix stack out-of-bounds write in cfctrl_link_setup()
Posted by Simon Horman 2 months ago 
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.
Re: [PATCH v5] net: caif: fix stack out-of-bounds write in cfctrl_link_setup()
Posted by Kangzheng Gu 1 month, 3 weeks ago 
Thanks for all of your advice, I am preparing a new version of patch now.

Simon Horman <horms@kernel.org> 于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.

Patchew 2.1-devel-ea86937

© 2016 - 2026 Red Hat, Inc.