isdn: Partially revert debug format string usage clean up

PATCH Applied Christoph Biedl 3 participants 3 messages Nov 25, 2015

Subsystems (2)

Networking Network drivers

lore

Share & tools

lore

Christoph Biedl

author

2015-11-25 06:56 UTC #0 lore raw

PATCH 1/1 isdn: Partially revert debug format string usage clean up

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 <[email protected]>

Cc Karsten Keil <[email protected]>

Signed-off-by Christoph Biedl <[email protected]>

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

drivers/isdn/hisax/config.c +1 -1

index b33f53b..bf04d2a 100644
@@ -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)",

drivers/isdn/hisax/hfc_pci.c +1 -1

index 4a48255..90449e1 100644
@@ -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);
 			}

drivers/isdn/hisax/hfc_sx.c +1 -1

index b1fad81..13b2151 100644
@@ -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);
 			}

drivers/isdn/hisax/q931.c +3 -3

index b420f8b..ba4beb2 100644
@@ -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);
 }

Signature · 1 line

2.1.4

David Miller

maintainer Applied

2015-11-25 16:50 UTC #1 lore raw

From: Christoph Biedl <[email protected]>
Date: Wed, 25 Nov 2015 07:47:40 +0100

Quoted 9 lines

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.

Quoted 6 lines

So revert the HiSax_putstatus invocations to the previous state.
Fixes 35a4a5733b0a ("isdn: clean up debug format string usage")
Cc Kees Cook <[email protected]>
Cc Karsten Keil <[email protected]>
Signed-off-by Christoph Biedl <[email protected]>

Applied, thanks.

Kees Cook

Acked-by on v1 1/1

2015-11-25 18:36 UTC #2 lore raw

In reply to Christoph Biedl

Quoted 8 lines · Christoph Biedl

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 <[email protected]>

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

Quoted 83 lines