| Message ID | 20210405195744.19386-1-paskripkin@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | net: fix shift-out-of-bounds in nl802154_new_interface | expand |
Hi, On Mon, 5 Apr 2021 at 15:58, Pavel Skripkin <paskripkin@gmail.com> wrote: > > syzbot reported shift-out-of-bounds in nl802154_new_interface. > The problem was in signed representation of enum nl802154_iftype > > enum nl802154_iftype { > /* for backwards compatibility TODO */ > NL802154_IFTYPE_UNSPEC = -1, > ... > > Since, enum has negative value in it, objects of this type > will be represented as signed integer. > > type = nla_get_u32(info->attrs[NL802154_ATTR_IFTYPE]); > > u32 will be casted to signed, which can cause negative value type. > > Reported-by: syzbot+7bf7b22759195c9a21e9@syzkaller.appspotmail.com > Signed-off-by: Pavel Skripkin <paskripkin@gmail.com> Yes, this patch will fix the issue but we discussed that the problem is deeper than such a fix. The real problem is that we are using a -1 value which doesn't fit into the u32 netlink value and it gets converted back and forward which we should avoid. - Alex
On Tue, 2021-04-06 at 08:21 -0400, Alexander Aring wrote: > Hi, > > On Mon, 5 Apr 2021 at 15:58, Pavel Skripkin <paskripkin@gmail.com> > wrote: > > > > syzbot reported shift-out-of-bounds in nl802154_new_interface. > > The problem was in signed representation of enum nl802154_iftype > > > > enum nl802154_iftype { > > /* for backwards compatibility TODO */ > > NL802154_IFTYPE_UNSPEC = -1, > > ... > > > > Since, enum has negative value in it, objects of this type > > will be represented as signed integer. > > > > type = nla_get_u32(info->attrs[NL802154_ATTR_IFTYPE]); > > > > u32 will be casted to signed, which can cause negative value type. > > > > Reported-by: syzbot+7bf7b22759195c9a21e9@syzkaller.appspotmail.com > > Signed-off-by: Pavel Skripkin <paskripkin@gmail.com> > > Yes, this patch will fix the issue but we discussed that the problem > is deeper than such a fix. The real problem is that we are using a -1 > value which doesn't fit into the u32 netlink value and it gets > converted back and forward which we should avoid. > OK, thanks for feedback! > > - Alex With regards, Pavel Skripkin
diff --git a/net/ieee802154/nl802154.c b/net/ieee802154/nl802154.c index 7c5a1aa5adb4..6cce045e3d40 100644 --- a/net/ieee802154/nl802154.c +++ b/net/ieee802154/nl802154.c @@ -910,7 +910,7 @@ static int nl802154_new_interface(struct sk_buff *skb, struct genl_info *info) if (info->attrs[NL802154_ATTR_IFTYPE]) { type = nla_get_u32(info->attrs[NL802154_ATTR_IFTYPE]); - if (type > NL802154_IFTYPE_MAX || + if (type > NL802154_IFTYPE_MAX || type < 0 || !(rdev->wpan_phy.supported.iftypes & BIT(type))) return -EINVAL; }
syzbot reported shift-out-of-bounds in nl802154_new_interface. The problem was in signed representation of enum nl802154_iftype enum nl802154_iftype { /* for backwards compatibility TODO */ NL802154_IFTYPE_UNSPEC = -1, ... Since, enum has negative value in it, objects of this type will be represented as signed integer. type = nla_get_u32(info->attrs[NL802154_ATTR_IFTYPE]); u32 will be casted to signed, which can cause negative value type. Reported-by: syzbot+7bf7b22759195c9a21e9@syzkaller.appspotmail.com Signed-off-by: Pavel Skripkin <paskripkin@gmail.com> --- net/ieee802154/nl802154.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)