Linux-WPAN Archive on lore.kernel.org
 help / color / Atom feed
From: Alexander Aring <alex.aring@gmail.com>
To: Stefan Schmidt <stefan@datenfreihafen.org>
Cc: Alexander Aring <aahringo@redhat.com>,
	linux-wpan - ML <linux-wpan@vger.kernel.org>,
	"open list:NETWORKING [GENERAL]" <netdev@vger.kernel.org>
Subject: Re: [PATCH wpan 01/17] net: ieee802154: make shift exponent unsigned
Date: Sat, 6 Mar 2021 18:35:06 -0500
Message-ID: <CAB_54W6d0NC7W3U6EMOR7RxYGQhJS_hAFcgRcrpM5W7BC7SXXg@mail.gmail.com> (raw)
In-Reply-To: <b9baaf49-0e25-4c74-e8b7-f826157e1d48@datenfreihafen.org>

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

  reply index

Thread overview: 25+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-02-28 15:18 [PATCH wpan 00/17] ieee802154: syzbot fixes Alexander Aring
2021-02-28 15:18 ` [PATCH wpan 01/17] net: ieee802154: make shift exponent unsigned Alexander Aring
2021-03-02 21:18   ` Stefan Schmidt
2021-03-06 23:35     ` Alexander Aring [this message]
2021-02-28 15:18 ` [PATCH wpan 02/17] net: ieee802154: fix memory leak when deliver monitor skbs Alexander Aring
2021-03-01  3:16   ` Alexander Aring
2021-03-02 12:43     ` Stefan Schmidt
2021-02-28 15:18 ` [PATCH wpan 03/17] net: ieee802154: nl-mac: fix check on panid Alexander Aring
2021-03-02 21:33   ` Stefan Schmidt
2021-02-28 15:18 ` [PATCH wpan 04/17] net: ieee802154: forbid monitor for set llsec params Alexander Aring
2021-03-02 21:45   ` Stefan Schmidt
2021-03-06 13:12     ` Alexander Aring
2021-02-28 15:18 ` [PATCH wpan 05/17] net: ieee802154: stop dump llsec keys for monitors Alexander Aring
2021-02-28 15:18 ` [PATCH wpan 06/17] net: ieee802154: forbid monitor for add llsec key Alexander Aring
2021-02-28 15:18 ` [PATCH wpan 07/17] net: ieee802154: forbid monitor for del " Alexander Aring
2021-02-28 15:18 ` [PATCH wpan 08/17] net: ieee802154: stop dump llsec devs for monitors Alexander Aring
2021-02-28 15:18 ` [PATCH wpan 09/17] net: ieee802154: forbid monitor for add llsec dev Alexander Aring
2021-02-28 15:18 ` [PATCH wpan 10/17] net: ieee802154: forbid monitor for del " Alexander Aring
2021-02-28 15:18 ` [PATCH wpan 11/17] net: ieee802154: stop dump llsec devkeys for monitors Alexander Aring
2021-02-28 15:18 ` [PATCH wpan 12/17] net: ieee802154: forbid monitor for add llsec devkey Alexander Aring
2021-02-28 15:18 ` [PATCH wpan 13/17] net: ieee802154: forbid monitor for del " Alexander Aring
2021-02-28 15:18 ` [PATCH wpan 14/17] net: ieee802154: stop dump llsec seclevels for monitors Alexander Aring
2021-02-28 15:18 ` [PATCH wpan 15/17] net: ieee802154: forbid monitor for add llsec seclevel Alexander Aring
2021-02-28 15:18 ` [PATCH wpan 16/17] net: ieee802154: forbid monitor for del " Alexander Aring
2021-02-28 15:18 ` [PATCH wpan 17/17] net: ieee802154: stop dump llsec params for monitors Alexander Aring

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAB_54W6d0NC7W3U6EMOR7RxYGQhJS_hAFcgRcrpM5W7BC7SXXg@mail.gmail.com \
    --to=alex.aring@gmail.com \
    --cc=aahringo@redhat.com \
    --cc=linux-wpan@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=stefan@datenfreihafen.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-WPAN Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-wpan/0 linux-wpan/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-wpan linux-wpan/ https://lore.kernel.org/linux-wpan \
		linux-wpan@vger.kernel.org
	public-inbox-index linux-wpan

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-wpan


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git