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
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
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
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]>
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
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
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