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

Simon Horman
2026-04-12T13:57:48Z

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).