| Message ID | 20210228151817.95700-2-aahringo@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ieee802154: syzbot fixes | expand |
Hello Alex. On 28.02.21 16:18, Alexander Aring wrote: > This patch changes the iftype type variable to unsigned that it can > never be reach a negative value. > > Reported-by: syzbot+7bf7b22759195c9a21e9@syzkaller.appspotmail.com > Signed-off-by: Alexander Aring <aahringo@redhat.com> > --- > net/ieee802154/nl802154.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/net/ieee802154/nl802154.c b/net/ieee802154/nl802154.c > index e9e4652cd592..3ee09f6d13b7 100644 > --- a/net/ieee802154/nl802154.c > +++ b/net/ieee802154/nl802154.c > @@ -898,8 +898,8 @@ static int nl802154_get_interface(struct sk_buff *skb, struct genl_info *info) > static int nl802154_new_interface(struct sk_buff *skb, struct genl_info *info) > { > struct cfg802154_registered_device *rdev = info->user_ptr[0]; > - enum nl802154_iftype type = NL802154_IFTYPE_UNSPEC; > __le64 extended_addr = cpu_to_le64(0x0000000000000000ULL); > + u32 type = NL802154_IFTYPE_UNSPEC; > > /* TODO avoid failing a new interface > * creation due to pending removal? > I am concerned about this one. Maybe you can shed some light on it. NL802154_IFTYPE_UNSPEC is -1 which means the u32 will not hold this value, but something at the end of the range for u32. There is a path (info->attrs[NL802154_ATTR_IFTYPE] is not true) where we put type forward to rdev_add_virtual_intf() with its changed value but it would expect and enum which could hold -1 for UNSPEC. regards Stefan Schmidt
Hi Stefan, On Thu, 4 Mar 2021 at 02:28, Stefan Schmidt <stefan@datenfreihafen.org> wrote: > > Hello Alex. > > On 28.02.21 16:18, Alexander Aring wrote: > > This patch changes the iftype type variable to unsigned that it can > > never be reach a negative value. > > > > Reported-by: syzbot+7bf7b22759195c9a21e9@syzkaller.appspotmail.com > > Signed-off-by: Alexander Aring <aahringo@redhat.com> > > --- > > net/ieee802154/nl802154.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/net/ieee802154/nl802154.c b/net/ieee802154/nl802154.c > > index e9e4652cd592..3ee09f6d13b7 100644 > > --- a/net/ieee802154/nl802154.c > > +++ b/net/ieee802154/nl802154.c > > @@ -898,8 +898,8 @@ static int nl802154_get_interface(struct sk_buff *skb, struct genl_info *info) > > static int nl802154_new_interface(struct sk_buff *skb, struct genl_info *info) > > { > > struct cfg802154_registered_device *rdev = info->user_ptr[0]; > > - enum nl802154_iftype type = NL802154_IFTYPE_UNSPEC; > > __le64 extended_addr = cpu_to_le64(0x0000000000000000ULL); > > + u32 type = NL802154_IFTYPE_UNSPEC; > > > > /* TODO avoid failing a new interface > > * creation due to pending removal? > > > > I am concerned about this one. Maybe you can shed some light on it. > NL802154_IFTYPE_UNSPEC is -1 which means the u32 will not hold this > value, but something at the end of the range for u32. > yes, ugh... it's NL802154_IFTYPE_UNSPEC = -1 only for NL802154_IFTYPE... all others UNSPEC are 0. There is a comment there /* for backwards compatibility TODO */. I think I did that because the old netlink interfaces and instead of mapping new values to old values (internally) which is bad. Would it be 0 I think the compiler would handle it as unsigned. > There is a path (info->attrs[NL802154_ATTR_IFTYPE] is not true) where we > put type forward to rdev_add_virtual_intf() with its changed value but > it would expect and enum which could hold -1 for UNSPEC. > It will be converted back here to -1 again? Or maybe depends on the compiler, because it may use a different int type which the enum values fits? I am not sure here... In nl802154 we use u32 (netlink) for enums because the range fits, however this isn't true for NL802154_IFTYPE_, we cannot change it back. I think we should try to switch NL802154_IFTYPE_UNSPEC to "(~(__u32)0)" and let start NL802154_IFTYPE_NODE = 0. Which is still backwards compatible. Just give the compiler a note to handle it as unsigned value and more importantly an enum where the range fits in. It depends on the compiler, may it decide to use a signed char for this enum, then we get problems when converting it ? After quick research it seems we can not rely on whatever the compiler handles the enum as signed or unsigned and that makes problems with the shift operator "BIT(type)" and it's what this patch is trying to fix. I would make two patches, one is making the nl802154.h changes and the other is this patch, should be fine to handle it as enum value when we did some max range checks. There is also a third patch to return -EINVAL earlier if type attr isn't given, I think it's nothing for stable. - Alex
diff --git a/net/ieee802154/nl802154.c b/net/ieee802154/nl802154.c index e9e4652cd592..3ee09f6d13b7 100644 --- a/net/ieee802154/nl802154.c +++ b/net/ieee802154/nl802154.c @@ -898,8 +898,8 @@ static int nl802154_get_interface(struct sk_buff *skb, struct genl_info *info) static int nl802154_new_interface(struct sk_buff *skb, struct genl_info *info) { struct cfg802154_registered_device *rdev = info->user_ptr[0]; - enum nl802154_iftype type = NL802154_IFTYPE_UNSPEC; __le64 extended_addr = cpu_to_le64(0x0000000000000000ULL); + u32 type = NL802154_IFTYPE_UNSPEC; /* TODO avoid failing a new interface * creation due to pending removal?
This patch changes the iftype type variable to unsigned that it can never be reach a negative value. Reported-by: syzbot+7bf7b22759195c9a21e9@syzkaller.appspotmail.com Signed-off-by: Alexander Aring <aahringo@redhat.com> --- net/ieee802154/nl802154.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)