* [PATCH net] isdn: Partially revert debug format string usage clean up
@ 2015-11-25 6:47 Christoph Biedl
2015-11-25 16:50 ` David Miller
2015-11-25 18:35 ` Kees Cook
0 siblings, 2 replies; 3+ messages in thread
From: Christoph Biedl @ 2015-11-25 6:47 UTC (permalink / raw)
To: netdev, Karsten Keil, Kees Cook
Commit 35a4a57 ("isdn: clean up debug format string usage") introduced
a safeguard to avoid accidential format string interpolation of data
when calling debugl1 or HiSax_putstatus. This did however not take into
account VHiSax_putstatus (called by HiSax_putstatus) does *not* call
vsprintf if the head parameter is NULL - the format string is treated
as plain text then instead. As a result, the string "%s" is processed
literally, and the actual information is lost. This affects the isdnlog
userspace program which stopped logging information since that commit.
So revert the HiSax_putstatus invocations to the previous state.
Fixes: 35a4a5733b0a ("isdn: clean up debug format string usage")
Cc: Kees Cook <keescook@chromium.org>
Cc: Karsten Keil <isdn@linux-pingi.de>
Signed-off-by: Christoph Biedl <linux-kernel.bfrz@manchmal.in-ulm.de>
---
drivers/isdn/hisax/config.c | 2 +-
drivers/isdn/hisax/hfc_pci.c | 2 +-
drivers/isdn/hisax/hfc_sx.c | 2 +-
drivers/isdn/hisax/q931.c | 6 +++---
4 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/isdn/hisax/config.c b/drivers/isdn/hisax/config.c
index b33f53b..bf04d2a 100644
--- a/drivers/isdn/hisax/config.c
+++ b/drivers/isdn/hisax/config.c
@@ -1896,7 +1896,7 @@ static void EChannel_proc_rcv(struct hisax_d_if *d_if)
ptr--;
*ptr++ = '\n';
*ptr = 0;
- HiSax_putstatus(cs, NULL, "%s", cs->dlog);
+ HiSax_putstatus(cs, NULL, cs->dlog);
} else
HiSax_putstatus(cs, "LogEcho: ",
"warning Frame too big (%d)",
diff --git a/drivers/isdn/hisax/hfc_pci.c b/drivers/isdn/hisax/hfc_pci.c
index 4a48255..90449e1 100644
--- a/drivers/isdn/hisax/hfc_pci.c
+++ b/drivers/isdn/hisax/hfc_pci.c
@@ -901,7 +901,7 @@ Begin:
ptr--;
*ptr++ = '\n';
*ptr = 0;
- HiSax_putstatus(cs, NULL, "%s", cs->dlog);
+ HiSax_putstatus(cs, NULL, cs->dlog);
} else
HiSax_putstatus(cs, "LogEcho: ", "warning Frame too big (%d)", total - 3);
}
diff --git a/drivers/isdn/hisax/hfc_sx.c b/drivers/isdn/hisax/hfc_sx.c
index b1fad81..13b2151 100644
--- a/drivers/isdn/hisax/hfc_sx.c
+++ b/drivers/isdn/hisax/hfc_sx.c
@@ -674,7 +674,7 @@ receive_emsg(struct IsdnCardState *cs)
ptr--;
*ptr++ = '\n';
*ptr = 0;
- HiSax_putstatus(cs, NULL, "%s", cs->dlog);
+ HiSax_putstatus(cs, NULL, cs->dlog);
} else
HiSax_putstatus(cs, "LogEcho: ", "warning Frame too big (%d)", skb->len);
}
diff --git a/drivers/isdn/hisax/q931.c b/drivers/isdn/hisax/q931.c
index b420f8b..ba4beb2 100644
--- a/drivers/isdn/hisax/q931.c
+++ b/drivers/isdn/hisax/q931.c
@@ -1179,7 +1179,7 @@ LogFrame(struct IsdnCardState *cs, u_char *buf, int size)
dp--;
*dp++ = '\n';
*dp = 0;
- HiSax_putstatus(cs, NULL, "%s", cs->dlog);
+ HiSax_putstatus(cs, NULL, cs->dlog);
} else
HiSax_putstatus(cs, "LogFrame: ", "warning Frame too big (%d)", size);
}
@@ -1246,7 +1246,7 @@ dlogframe(struct IsdnCardState *cs, struct sk_buff *skb, int dir)
}
if (finish) {
*dp = 0;
- HiSax_putstatus(cs, NULL, "%s", cs->dlog);
+ HiSax_putstatus(cs, NULL, cs->dlog);
return;
}
if ((0xfe & buf[0]) == PROTO_DIS_N0) { /* 1TR6 */
@@ -1509,5 +1509,5 @@ dlogframe(struct IsdnCardState *cs, struct sk_buff *skb, int dir)
dp += sprintf(dp, "Unknown protocol %x!", buf[0]);
}
*dp = 0;
- HiSax_putstatus(cs, NULL, "%s", cs->dlog);
+ HiSax_putstatus(cs, NULL, cs->dlog);
}
--
2.1.4
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH net] isdn: Partially revert debug format string usage clean up
2015-11-25 6:47 [PATCH net] isdn: Partially revert debug format string usage clean up Christoph Biedl
@ 2015-11-25 16:50 ` David Miller
2015-11-25 18:35 ` Kees Cook
1 sibling, 0 replies; 3+ messages in thread
From: David Miller @ 2015-11-25 16:50 UTC (permalink / raw)
To: linux-kernel.bfrz; +Cc: netdev, isdn, keescook
From: Christoph Biedl <linux-kernel.bfrz@manchmal.in-ulm.de>
Date: Wed, 25 Nov 2015 07:47:40 +0100
> Commit 35a4a57 ("isdn: clean up debug format string usage") introduced
> a safeguard to avoid accidential format string interpolation of data
> when calling debugl1 or HiSax_putstatus. This did however not take into
> account VHiSax_putstatus (called by HiSax_putstatus) does *not* call
> vsprintf if the head parameter is NULL - the format string is treated
> as plain text then instead. As a result, the string "%s" is processed
> literally, and the actual information is lost. This affects the isdnlog
> userspace program which stopped logging information since that commit.
>
> So revert the HiSax_putstatus invocations to the previous state.
>
> Fixes: 35a4a5733b0a ("isdn: clean up debug format string usage")
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Karsten Keil <isdn@linux-pingi.de>
> Signed-off-by: Christoph Biedl <linux-kernel.bfrz@manchmal.in-ulm.de>
Applied, thanks.
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH net] isdn: Partially revert debug format string usage clean up
2015-11-25 6:47 [PATCH net] isdn: Partially revert debug format string usage clean up Christoph Biedl
2015-11-25 16:50 ` David Miller
@ 2015-11-25 18:35 ` Kees Cook
1 sibling, 0 replies; 3+ messages in thread
From: Kees Cook @ 2015-11-25 18:35 UTC (permalink / raw)
To: Christoph Biedl; +Cc: Network Development, Karsten Keil
On Tue, Nov 24, 2015 at 10:47 PM, Christoph Biedl
<linux-kernel.bfrz@manchmal.in-ulm.de> wrote:
> Commit 35a4a57 ("isdn: clean up debug format string usage") introduced
> a safeguard to avoid accidential format string interpolation of data
> when calling debugl1 or HiSax_putstatus. This did however not take into
> account VHiSax_putstatus (called by HiSax_putstatus) does *not* call
> vsprintf if the head parameter is NULL - the format string is treated
> as plain text then instead. As a result, the string "%s" is processed
> literally, and the actual information is lost. This affects the isdnlog
> userspace program which stopped logging information since that commit.
Oh, that's weird, but yeah, these calls aren't expanding format
strings, so I'm fine with this change.
Acked-by: Kees Cook <keescook@chromium.org>
On the other hand, VHiSax_putstatus contains an unchecked data buffer
overflow, since it uses tmpbuf without checking lengths at all:
p = tmpbuf;
if (head) {
p += jiftime(p, jiffies);
p += sprintf(p, " %s", head);
p += vsprintf(p, fmt, args);
*p++ = '\n';
*p = 0;
len = p - tmpbuf;
p = tmpbuf;
} else {
p = fmt;
len = strlen(fmt);
}
if (len > HISAX_STATUS_BUFSIZE) {
spin_unlock_irqrestore(&cs->statlock, flags);
printk(KERN_WARNING "HiSax: status overflow %d/%d\n",
len, HISAX_STATUS_BUFSIZE);
return;
}
It helpfully detects the overflow, but at this point it's too late:
the buffer (and things past it) have already been clobbered. Seems
reachable through capi_debug, callc_debug, but nothing jumps out at me
as passing taking too-long arguments, though there are some long call
chains that touch ioctls and tty stuff, which seems scary. :)
Also, though mitigated by needing DEB_DLOG_VERBOSE, there seems to be
and overflow in dlogframe too: it doesn't validate against
MAX_DLOG_SPACE. For example path:
GROUP_TEI
CTRL_SAPI
ftyp == 3
while ... adds to dp for length of skb
... heap buffer overflow
-Kees
>
> Fixes: 35a4a5733b0a ("isdn: clean up debug format string usage")
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Karsten Keil <isdn@linux-pingi.de>
> Signed-off-by: Christoph Biedl <linux-kernel.bfrz@manchmal.in-ulm.de>
> ---
> drivers/isdn/hisax/config.c | 2 +-
> drivers/isdn/hisax/hfc_pci.c | 2 +-
> drivers/isdn/hisax/hfc_sx.c | 2 +-
> drivers/isdn/hisax/q931.c | 6 +++---
> 4 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/isdn/hisax/config.c b/drivers/isdn/hisax/config.c
> index b33f53b..bf04d2a 100644
> --- a/drivers/isdn/hisax/config.c
> +++ b/drivers/isdn/hisax/config.c
> @@ -1896,7 +1896,7 @@ static void EChannel_proc_rcv(struct hisax_d_if *d_if)
> ptr--;
> *ptr++ = '\n';
> *ptr = 0;
> - HiSax_putstatus(cs, NULL, "%s", cs->dlog);
> + HiSax_putstatus(cs, NULL, cs->dlog);
> } else
> HiSax_putstatus(cs, "LogEcho: ",
> "warning Frame too big (%d)",
> diff --git a/drivers/isdn/hisax/hfc_pci.c b/drivers/isdn/hisax/hfc_pci.c
> index 4a48255..90449e1 100644
> --- a/drivers/isdn/hisax/hfc_pci.c
> +++ b/drivers/isdn/hisax/hfc_pci.c
> @@ -901,7 +901,7 @@ Begin:
> ptr--;
> *ptr++ = '\n';
> *ptr = 0;
> - HiSax_putstatus(cs, NULL, "%s", cs->dlog);
> + HiSax_putstatus(cs, NULL, cs->dlog);
> } else
> HiSax_putstatus(cs, "LogEcho: ", "warning Frame too big (%d)", total - 3);
> }
> diff --git a/drivers/isdn/hisax/hfc_sx.c b/drivers/isdn/hisax/hfc_sx.c
> index b1fad81..13b2151 100644
> --- a/drivers/isdn/hisax/hfc_sx.c
> +++ b/drivers/isdn/hisax/hfc_sx.c
> @@ -674,7 +674,7 @@ receive_emsg(struct IsdnCardState *cs)
> ptr--;
> *ptr++ = '\n';
> *ptr = 0;
> - HiSax_putstatus(cs, NULL, "%s", cs->dlog);
> + HiSax_putstatus(cs, NULL, cs->dlog);
> } else
> HiSax_putstatus(cs, "LogEcho: ", "warning Frame too big (%d)", skb->len);
> }
> diff --git a/drivers/isdn/hisax/q931.c b/drivers/isdn/hisax/q931.c
> index b420f8b..ba4beb2 100644
> --- a/drivers/isdn/hisax/q931.c
> +++ b/drivers/isdn/hisax/q931.c
> @@ -1179,7 +1179,7 @@ LogFrame(struct IsdnCardState *cs, u_char *buf, int size)
> dp--;
> *dp++ = '\n';
> *dp = 0;
> - HiSax_putstatus(cs, NULL, "%s", cs->dlog);
> + HiSax_putstatus(cs, NULL, cs->dlog);
> } else
> HiSax_putstatus(cs, "LogFrame: ", "warning Frame too big (%d)", size);
> }
> @@ -1246,7 +1246,7 @@ dlogframe(struct IsdnCardState *cs, struct sk_buff *skb, int dir)
> }
> if (finish) {
> *dp = 0;
> - HiSax_putstatus(cs, NULL, "%s", cs->dlog);
> + HiSax_putstatus(cs, NULL, cs->dlog);
> return;
> }
> if ((0xfe & buf[0]) == PROTO_DIS_N0) { /* 1TR6 */
> @@ -1509,5 +1509,5 @@ dlogframe(struct IsdnCardState *cs, struct sk_buff *skb, int dir)
> dp += sprintf(dp, "Unknown protocol %x!", buf[0]);
> }
> *dp = 0;
> - HiSax_putstatus(cs, NULL, "%s", cs->dlog);
> + HiSax_putstatus(cs, NULL, cs->dlog);
> }
> --
> 2.1.4
>
--
Kees Cook
Chrome OS & Brillo Security
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2015-11-25 18:36 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-11-25 6:47 [PATCH net] isdn: Partially revert debug format string usage clean up Christoph Biedl
2015-11-25 16:50 ` David Miller
2015-11-25 18:35 ` Kees Cook
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).