Re: [PATCH wpan 01/17] net: ieee802154: make shift exponent unsigned

[Alexander Aring <alex.aring@gmail.com>]

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