Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Hawkins Jiawei <yin31149@gmail.com>
To:     keescook@chromium.org
Cc:     18801353760@163.com, davem@davemloft.net, edumazet@google.com,
        johannes@sipsolutions.net, kuba@kernel.org,
        linux-kernel@vger.kernel.org, linux-wireless@vger.kernel.org,
        netdev@vger.kernel.org, pabeni@redhat.com, sfr@canb.auug.org.au,
        syzbot+473754e5af963cf014cf@syzkaller.appspotmail.com,
        syzkaller-bugs@googlegroups.com, yin31149@gmail.com
Subject: Re: [PATCH] Add linux-next specific files for 20220923
Date:   Sun, 25 Sep 2022 14:25:01 +0800
Message-Id: <20220925062501.4373-1-yin31149@gmail.com>
In-Reply-To: <202209240905.F5654D7A5@keescook>
References: <202209240905.F5654D7A5@keescook>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

On Sun, 25 Sept 2022 at 00:26, Kees Cook <keescook@chromium.org> wrote:
>
> On Sat, Sep 24, 2022 at 11:55:14PM +0800, Hawkins Jiawei wrote:
> > > And as for the value of offsetof in calculating **offset**,
> > > I wonder if we can use the macro defined in
> > > include/linux/wireless.h as below, which makes code simplier:
> > > #define IW_EV_COMPAT_LCP_LEN offsetof(struct __compat_iw_event, pointer)
>
> Ah yes, that would be good.
>
> > According to above code, it seems that kernel will saves enough memory
> > (hdr_len + extra_len bytes) for payload structure in
> > nla_reserve()(Please correct me if I am wrong), pointed by compat_event.
> > So I wonder if we can use unsafe_memcpy(), to avoid unnecessary
> > memcpy() check as below, which seems more simple:
>
> I'd rather this was properly resolved with the creation of a real
> flexible array so that when bounds tracking gets improved in the future,
> the compiler can reason about it better. And, I think, it makes the code
> more readable:
>
> diff --git a/include/linux/wireless.h b/include/linux/wireless.h
> index 2d1b54556eff..e0b4b46da63f 100644
> --- a/include/linux/wireless.h
> +++ b/include/linux/wireless.h
> @@ -26,7 +26,10 @@ struct compat_iw_point {
>  struct __compat_iw_event {
>         __u16           len;                    /* Real length of this stuff */
>         __u16           cmd;                    /* Wireless IOCTL */
> -       compat_caddr_t  pointer;
> +       union {
> +               compat_caddr_t  pointer;
> +               DECLARE_FLEX_ARRAY(__u8, ptr_bytes);
> +       };
>  };
>  #define IW_EV_COMPAT_LCP_LEN offsetof(struct __compat_iw_event, pointer)
>  #define IW_EV_COMPAT_POINT_OFF offsetof(struct compat_iw_point, length)
> diff --git a/net/wireless/wext-core.c b/net/wireless/wext-core.c
> index 76a80a41615b..6079c8f4b634 100644
> --- a/net/wireless/wext-core.c
> +++ b/net/wireless/wext-core.c
> @@ -468,6 +468,7 @@ void wireless_send_event(struct net_device *        dev,
>         struct __compat_iw_event *compat_event;
>         struct compat_iw_point compat_wrqu;
>         struct sk_buff *compskb;
> +       int ptr_len;
>  #endif
>
>         /*
> @@ -582,6 +583,7 @@ void wireless_send_event(struct net_device *        dev,
>         nlmsg_end(skb, nlh);
>  #ifdef CONFIG_COMPAT
>         hdr_len = compat_event_type_size[descr->header_type];
> +       ptr_len = hdr_len - IW_EV_COMPAT_LCP_LEN;
>         event_len = hdr_len + extra_len;
>
>         compskb = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_ATOMIC);
> @@ -612,16 +614,15 @@ void wireless_send_event(struct net_device *      dev,
>         if (descr->header_type == IW_HEADER_TYPE_POINT) {
>                 compat_wrqu.length = wrqu->data.length;
>                 compat_wrqu.flags = wrqu->data.flags;
> -               memcpy(&compat_event->pointer,
> +               memcpy(compat_event->ptr_bytes,
>                         ((char *) &compat_wrqu) + IW_EV_COMPAT_POINT_OFF,
> -                       hdr_len - IW_EV_COMPAT_LCP_LEN);
> +                       ptr_len);
>                 if (extra_len)
> -                       memcpy(((char *) compat_event) + hdr_len,
> +                       memcpy(compat_event->ptr_bytes + ptr_len,
>                                 extra, extra_len);
>         } else {
>                 /* extra_len must be zero, so no if (extra) needed */
> -               memcpy(&compat_event->pointer, wrqu,
> -                       hdr_len - IW_EV_COMPAT_LCP_LEN);
> +               memcpy(compat_event->ptr_bytes, wrqu, ptr_len);
>         }
>
>         nlmsg_end(compskb, nlh);
>
>
> --
> Kees Cook
Yes, it seems that this code is more readable. I will refactor the patch
in this way, with some comments on union in struct compat_iw_point.

By the way, do you think we need to refactor the **struct compat_iw_point**
into union with some comments, or just replace the **pointer** field with
**DECLARE_FLEX_ARRAY(__u8, ptr_bytes)**? Because it seems that this field is
only used in wireless_send_event() and some macros in this
include/linux/wireless.h