In isdn_ppp_write(), the header (i.e., protobuf) of the buffer is fetched
twice from userspace. The first fetch is used to peek at the protocol
of the message and reset the huptimer if necessary; while the second
fetch copies in the whole buffer. However, given that buf resides in
userspace memory, a user process can race to change its memory content
across fetches. By doing so, we can either avoid resetting the huptimer
for any type of packets (by first setting proto to PPP_LCP and later
change to the actual type) or force resetting the huptimer for LCP packets.
This patch does a memcmp between the two fetches and abort if changes to
the protobuf is detected across fetches.
Signed-off-by: Meng Xu <[email protected]>
---
drivers/isdn/i4l/isdn_ppp.c | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/drivers/isdn/i4l/isdn_ppp.c b/drivers/isdn/i4l/isdn_ppp.c
index 6c44609..21a9ae8 100644
--- a/drivers/isdn/i4l/isdn_ppp.c
+++ b/drivers/isdn/i4l/isdn_ppp.c
@@ -857,6 +857,7 @@ isdn_ppp_write(int min, struct file *file, const char __user *buf, int count)
(lp->flags & ISDN_NET_CONNECTED)) {
unsigned short hl;
struct sk_buff *skb;
+ void *skb_tail;
/*
* we need to reserve enough space in front of
* sk_buff. old call to dev_alloc_skb only reserved
@@ -869,11 +870,21 @@ isdn_ppp_write(int min, struct file *file, const char __user *buf, int count)
return count;
}
skb_reserve(skb, hl);
- if (copy_from_user(skb_put(skb, count), buf, count))
+ skb_tail = skb_put(skb, count);
+ if (copy_from_user(skb_tail, buf, count))
{
kfree_skb(skb);
return -EFAULT;
}
+
+ /*
+ * abort if the message proto is changed between the fetches
+ */
+ if (memcmp(skb_tail, protobuf, 4)) {
+ kfree_skb(skb);
+ return -EFAULT;
+ }
+
if (is->debug & 0x40) {
printk(KERN_DEBUG "ppp xmit: len %d\n", (int) skb->len);
isdn_ppp_frame_log("xmit", skb->data, skb->len, 32, is->unit, lp->ppp_slot);
--
2.7.4
From: Meng Xu <[email protected]>
Date: Tue, 19 Sep 2017 14:52:58 -0400
> In isdn_ppp_write(), the header (i.e., protobuf) of the buffer is fetched
> twice from userspace. The first fetch is used to peek at the protocol
> of the message and reset the huptimer if necessary; while the second
> fetch copies in the whole buffer. However, given that buf resides in
> userspace memory, a user process can race to change its memory content
> across fetches. By doing so, we can either avoid resetting the huptimer
> for any type of packets (by first setting proto to PPP_LCP and later
> change to the actual type) or force resetting the huptimer for LCP packets.
>
> This patch does a memcmp between the two fetches and abort if changes to
> the protobuf is detected across fetches.
>
> Signed-off-by: Meng Xu <[email protected]>
Doing a memcmp() for every buffer is expensive, ugly, and not the
way we usually handle this kind of issue.
Instead, atomically copy the entire buffer, as needed.
Something like:
struct sk_buff *skb = NULL;
unsigned char protobuf[4];
unsigned char *cpy_buf;
if (lp->isdn_device >= 0 && lp->isdn_channel >= 0 &&
(dev->drv[lp->isdn_device]->flags & DRV_FLAG_RUNNING) &&
lp->dialstate == 0 &&
(lp->flags & ISDN_NET_CONNECTED)) {
/*
* we need to reserve enough space in front of
* sk_buff. old call to dev_alloc_skb only reserved
* 16 bytes, now we are looking what the driver want
*/
hl = dev->drv[lp->isdn_device]->interface->hl_hdrlen;
skb = alloc_skb(hl + count, GFP_ATOMIC);
if (!skb) {
printk(KERN_WARNING "isdn_ppp_write: out of memory!\n");
return count;
}
skb_reserve(skb, hl);
cpy_buf = skb_put(skb, count);
} else {
cpy_buf = protobuf;
count = sizeof(protobuf);
}
if (copy_from_user(cpy_buf, buf, count)) {
kfree_skb(skb);
return -EFAULT;
}
proto = PPP_PROTOCOL(cpy_buf);
if (proto != PPP_LCP)
lp->huptimer = 0;
...