Re: [patch] isdn: icn: buffer overflow in icn_command()

[Joe Perches <joe@perches.com>]

On Mon, 2014-04-14 at 11:07 +0300, Dan Carpenter wrote:
> The cbuf[] buffer is 60 characters but we're putting a potential 79
> characters and a NUL into it.  I've made it 80 characters and changed
> the sprintf() to snprintf().
[]
> @@ -1309,7 +1309,6 @@ icn_command(isdn_ctrl *c, icn_card *card)
>  			break;
>  		if ((c->arg & 255) < ICN_BCH) {
>  			char *p;
> -			char dial[50];
>  			char dcode[4];

The change log does not mentioned removal of dial.

As this subsystem likely has 0 active users, perhaps
making the minimal correctness change might be better.

Also, if making other changes, perhaps it'd be better
to similarly replace dcode with a pointer as well.

> @@ -1321,10 +1320,10 @@ icn_command(isdn_ctrl *c, icn_card *card)
>  			} else
>  				/* Normal Dial */
>  				strcpy(dcode, "CAL");
> -			strcpy(dial, p);
> -			sprintf(cbuf, "%02d;D%s_R%s,%02d,%02d,%s\n", (int) (a + 1),
> -				dcode, dial, c->parm.setup.si1,
> -				c->parm.setup.si2, c->parm.setup.eazmsn);
> +			snprintf(cbuf, sizeof(cbuf),
> +				 "%02d;D%s_R%s,%02d,%02d,%s\n", (int) (a + 1),
> +				 dcode, p, c->parm.setup.si1,
> +				 c->parm.setup.si2, c->parm.setup.eazmsn);
>  			i = icn_writecmd(cbuf, strlen(cbuf), 0, card);

Maybe save the snprintf result length and use it
in icn_writecmd too.