> Hi,
>
> On Sat, 24 Sept 2022 at 19:44, Hawkins Jiawei <
yin3...@gmail.com> wrote:
>>
>> Hi Kees,
>> I think this is dynamically sized.
>>
>> hdr_len = compat_event_type_size[descr->header_type];
>> event_len = hdr_len + extra_len;
>>
>> [...]
>>
>> /* Add the wireless events in the netlink packet */
>> nla = nla_reserve(compskb, IFLA_WIRELESS, event_len);
>> if (!nla) {
>> kfree_skb(skb);
>> kfree_skb(compskb);
>> return;
>> }
>> compat_event = nla_data(nla);
>>
>> [...]
>>
>> if (descr->header_type == IW_HEADER_TYPE_POINT) {
>> compat_wrqu.length = wrqu->data.length;
>> compat_wrqu.flags = wrqu->data.flags;
>> memcpy(&compat_event->pointer,
>> ((char *) &compat_wrqu) + IW_EV_COMPAT_POINT_OFF,
>> hdr_len - IW_EV_COMPAT_LCP_LEN);
>> if (extra_len)
>> memcpy(((char *) compat_event) + hdr_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);
>> }
>>
>> according to the above code, it seems that this structure is used to
>> parse ths payload from buffer, so the field **pointer** should just
>> be a position label to the unused bytes in buffer. Its unused bytes will be
>> parsed as different structure according to event type.
>>
>> >
>> > > };
>> > > #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..9d0b50abbe09 100644
>> > > --- a/net/wireless/wext-core.c
>> > > +++ b/net/wireless/wext-core.c
>> > > @@ -620,7 +620,7 @@ void wireless_send_event(struct net_device * dev,
>> >
>> > adding in more context code:
>> >
>> > memcpy(&compat_event->pointer,
>> > ((char *) &compat_wrqu) + IW_EV_COMPAT_POINT_OFF,
>> > hdr_len - IW_EV_COMPAT_LCP_LEN);
>> > if (extra_len)
>> > memcpy(((char *) compat_event) + hdr_len,
>> > extra, extra_len);
>> >
>> > The code above has "pointer" as a memcpy destination as well. I think
>> > that should be changed to pointer_flex as well, as the length calculation
>> > is the same. I wonder what FORTIFY will think about the second memcpy
>> > above. If I'm reading the math correctly, it might need to be:
>> >
>> > if (extra_len) {
>> > size_t offset = hdr_len - offsetof(typeof(*compat_event), pointer_flex);
>> > memcpy(&compat_event->pointer_flex[offset], extra, extra_len);
>> > }
>> >
>> I agree with you. It seems that in this situation,
>> the event type has been cleared, the unuesd bytes start from **pointer**
>> field should be parsed as struct iw_point type as below, which is a bigger
>> structure than **pointer**, it will also triggers the memcpy() warning.
>> /*
>> * The problem for 64/32 bit.
>> *
>> * On 64-bit, a regular event is laid out as follows:
>> * An iw_point event is laid out like this instead:
>> * | 0 | 1 | 2 | 3 | 4 | 5 | 6 | 7 |
>> * | event.len | event.cmd | p a d d i n g |
>> * | iwpnt.len | iwpnt.flg | p a d d i n g |
>> * | extra data ...
>> *
>> * The second padding exists because struct iw_point is extended,
>> * but this depends on the platform...
>> *
>> * On 32-bit, all the padding shouldn't be there.
>> */
>>
>> 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)
>>
>>
>> >
>> > > } else {
>> > > /* extra_len must be zero, so no if (extra) needed */
>> > > - memcpy(&compat_event->pointer, wrqu,
>> > > + memcpy(&compat_event->pointer_flex, wrqu,
>> > > hdr_len - IW_EV_COMPAT_LCP_LEN);
>> > > }
>> > >
>> >
>> > But otherwise, yes, looks like the right modification. Thanks for tackling
>> > this! It is quite a weird structure! :)
>> >
>> > -Kees
>> >
>> > --
>> > Kees Cook
>
> hdr_len = compat_event_type_size[descr->header_type];
> event_len = hdr_len + extra_len;
>
> [...]
>
> /* Add the wireless events in the netlink packet */
> nla = nla_reserve(compskb, IFLA_WIRELESS, event_len);
> if (!nla) {
> kfree_skb(skb);
> kfree_skb(compskb);
> return;
> }
> compat_event = nla_data(nla);
>
> if (descr->header_type == IW_HEADER_TYPE_POINT) {
> [...]
> memcpy(&compat_event->pointer,
> ((char *) &compat_wrqu) + IW_EV_COMPAT_POINT_OFF,
> hdr_len - IW_EV_COMPAT_LCP_LEN);
> if (extra_len)
> memcpy(((char *) compat_event) + hdr_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);
> }
>
>
> 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
> memory() check as below, which seems more simple:
>
> #syz test git://
git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
want 2 args (repo, branch), got 5
>
> diff --git a/net/wireless/wext-core.c b/net/wireless/wext-core.c
> index 76a80a41615b..a967da647e2b 100644
> --- a/net/wireless/wext-core.c
> +++ b/net/wireless/wext-core.c
> @@ -609,19 +609,26 @@ void wireless_send_event(struct net_device * dev,
>
> compat_event->len = event_len;
> compat_event->cmd = cmd;
> +
> + /* kernel reserves event_len's bytes for compat_event,
> + * so we don't need memcpy()'s bounds check
> + */
> if (descr->header_type == IW_HEADER_TYPE_POINT) {
> compat_wrqu.length = wrqu->data.length;
> compat_wrqu.flags = wrqu->data.flags;
> - memcpy(&compat_event->pointer,
> + unsafe_memcpy(&compat_event->pointer,
> ((char *) &compat_wrqu) + IW_EV_COMPAT_POINT_OFF,
> - hdr_len - IW_EV_COMPAT_LCP_LEN);
> + hdr_len - IW_EV_COMPAT_LCP_LEN,
> + /* compat_event has enough room */);
> if (extra_len)
> - memcpy(((char *) compat_event) + hdr_len,
> - extra, extra_len);
> + unsafe_memcpy(((char *) compat_event) + hdr_len,
> + extra, extra_len,
> + /* compat_event has enough room */);
> } else {
> /* extra_len must be zero, so no if (extra) needed */
> - memcpy(&compat_event->pointer, wrqu,
> - hdr_len - IW_EV_COMPAT_LCP_LEN);
> + unsafe_memcpy(&compat_event->pointer, wrqu,
> + hdr_len - IW_EV_COMPAT_LCP_LEN,
> + /* compat_event has enough room */);
> }
>
> nlmsg_end(compskb, nlh);