2014-11-09 22:44:05

by David Fries

Subject: [PATHC 0/2] cn: w1: buffer size checks

These issues were found by Dan Carpenter with a static checker.
Evgeniy are you okay with these?

[PATCH 1/2] cn: verify msg->len before making callback
[PATCH 2/2] w1: avoid potential u16 overflow

2014-11-09 22:44:09

by David Fries

Subject: [PATCH 1/2] cn: verify msg->len before making callback

The struct cn_msg len field comes from userspace and needs to be
validated. More logical to do so here where the cn_msg pointer is
pulled out of the sk_buff than the callback which is passed cn_msg *
and might assume no validation is needed.

Reported-by: Dan Carpenter <[email protected]>
Signed-off-by: David Fries <[email protected]>
---
drivers/connector/connector.c | 6 ++++++
1 file changed, 6 insertions(+)

diff --git a/drivers/connector/connector.c b/drivers/connector/connector.c
index f612d68..30f5228 100644
--- a/drivers/connector/connector.c
+++ b/drivers/connector/connector.c
@@ -141,12 +141,18 @@ EXPORT_SYMBOL_GPL(cn_netlink_send);
*/
static int cn_call_callback(struct sk_buff *skb)
{
+ struct nlmsghdr *nlh;
struct cn_callback_entry *i, *cbq = NULL;
struct cn_dev *dev = &cdev;
struct cn_msg *msg = nlmsg_data(nlmsg_hdr(skb));
struct netlink_skb_parms *nsp = &NETLINK_CB(skb);
int err = -ENODEV;

+ /* verify msg->len is within skb */
+ nlh = nlmsg_hdr(skb);
+ if (nlh->nlmsg_len < NLMSG_HDRLEN + sizeof(struct cn_msg) + msg->len)
+ return -EINVAL;
+
spin_lock_bh(&dev->cbdev->queue_lock);
list_for_each_entry(i, &dev->cbdev->queue_list, callback_entry) {
if (cn_cb_equal(&i->id.id, &msg->id)) {
--
1.7.10.4

2014-11-09 22:44:34

by David Fries

Subject: [PATCH 2/2] w1: avoid potential u16 overflow

Reported-by: Dan Carpenter <[email protected]>
Signed-off-by: David Fries <[email protected]>
---
drivers/w1/w1_netlink.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/w1/w1_netlink.c b/drivers/w1/w1_netlink.c
index dd96562..881597a 100644
--- a/drivers/w1/w1_netlink.c
+++ b/drivers/w1/w1_netlink.c
@@ -598,7 +598,7 @@ static void w1_cn_callback(struct cn_msg *cn, struct netlink_skb_parms *nsp)
msg = (struct w1_netlink_msg *)(cn + 1);
if (node_count) {
int size;
- u16 reply_size = sizeof(*cn) + cn->len + slave_len;
+ int reply_size = sizeof(*cn) + cn->len + slave_len;
if (cn->flags & W1_CN_BUNDLE) {
/* bundling duplicats some of the messages */
reply_size += 2 * cmd_count * (sizeof(struct cn_msg) +
--
1.7.10.4

2014-11-10 00:12:35

by Evgeniy Polyakov

Subject: Re: [PATHC 0/2] cn: w1: buffer size checks

Hi

10.11.2014, 01:37, "David Fries" <[email protected]>:
> These issues were found by Dan Carpenter with a static checker.
> Evgeniy are you okay with these?
>
> [PATCH 1/2] cn: verify msg->len before making callback
> [PATCH 2/2] w1: avoid potential u16 overflow

Both patches look good, thank you
Acked-by: Evgeniy Polyakov <[email protected]>

2014-11-11 02:20:17

by David Fries

Subject: [PATHC 0/2] cn: w1: buffer size checks

Greg Kroah-Hartman,
These issues were found by Dan Carpenter with a static checker and will check
message buffer lengths from userspace or avoid length overflows. Evgeniy
Polyakov has given his ack, and they can be applied to the stable branch as
well.

[PATCH 1/2] cn: verify msg->len before making callback
[PATCH 2/2] w1: avoid potential u16 overflow

2014-11-11 02:21:07

by David Fries

Subject: [PATCH 1/2] cn: verify msg->len before making callback

The struct cn_msg len field comes from userspace and needs to be
validated. More logical to do so here where the cn_msg pointer is
pulled out of the sk_buff than the callback which is passed cn_msg *
and might assume no validation is needed.

Reported-by: Dan Carpenter <[email protected]>
Acked-by: Evgeniy Polyakov <[email protected]>
Signed-off-by: David Fries <[email protected]>
---
drivers/connector/connector.c | 6 ++++++
1 file changed, 6 insertions(+)

diff --git a/drivers/connector/connector.c b/drivers/connector/connector.c
index f612d68..30f5228 100644
--- a/drivers/connector/connector.c
+++ b/drivers/connector/connector.c
@@ -141,12 +141,18 @@ EXPORT_SYMBOL_GPL(cn_netlink_send);
*/
static int cn_call_callback(struct sk_buff *skb)
{
+ struct nlmsghdr *nlh;
struct cn_callback_entry *i, *cbq = NULL;
struct cn_dev *dev = &cdev;
struct cn_msg *msg = nlmsg_data(nlmsg_hdr(skb));
struct netlink_skb_parms *nsp = &NETLINK_CB(skb);
int err = -ENODEV;

+ /* verify msg->len is within skb */
+ nlh = nlmsg_hdr(skb);
+ if (nlh->nlmsg_len < NLMSG_HDRLEN + sizeof(struct cn_msg) + msg->len)
+ return -EINVAL;
+
spin_lock_bh(&dev->cbdev->queue_lock);
list_for_each_entry(i, &dev->cbdev->queue_list, callback_entry) {
if (cn_cb_equal(&i->id.id, &msg->id)) {
--
1.7.10.4

2014-11-11 02:21:54

by David Fries

Subject: [PATCH 2/2] w1: avoid potential u16 overflow

Reported-by: Dan Carpenter <[email protected]>
Acked-by: Evgeniy Polyakov <[email protected]>
Signed-off-by: David Fries <[email protected]>
---
drivers/w1/w1_netlink.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/w1/w1_netlink.c b/drivers/w1/w1_netlink.c
index dd96562..881597a 100644
--- a/drivers/w1/w1_netlink.c
+++ b/drivers/w1/w1_netlink.c
@@ -598,7 +598,7 @@ static void w1_cn_callback(struct cn_msg *cn, struct netlink_skb_parms *nsp)
msg = (struct w1_netlink_msg *)(cn + 1);
if (node_count) {
int size;
- u16 reply_size = sizeof(*cn) + cn->len + slave_len;
+ int reply_size = sizeof(*cn) + cn->len + slave_len;
if (cn->flags & W1_CN_BUNDLE) {
/* bundling duplicats some of the messages */
reply_size += 2 * cmd_count * (sizeof(struct cn_msg) +
--
1.7.10.4