netdev - Re: [PATCH] net: mroute6.h: change type of mif6c_pifi to _

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <PAZP264MB406414BA18689729DDE24F3DFC659@PAZP264MB4064.FRAP264.PROD.OUTLOOK.COM>
Date:   Tue, 2 May 2023 08:07:10 +0000
From:   Ganesh Babu <ganesh.babu@...nops.com>
To:     Jakub Kicinski <kuba@...nel.org>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Ganesh Babu <ganesh.babu@...nops.com>
Subject: Re: [PATCH] net: mroute6.h: change type of mif6c_pifi to __u32

Thank you for your response. Regarding the proposed change to
the mif6ctl structure in mroute6.h, I would like to clarify,
that changing the datatype of mif6c_pifi from __u16 to __u32
will not change the offset of the structure members, which
means that the size of the structure remains the same and
the ABI remains compatible. Furthermore, ifindex is treated
as an integer in all the subsystems of the kernel and not
as a 16-bit value. Therefore, changing the datatype of
mif6c_pifi from __u16 to __u32 is a natural and expected
change that aligns with the existing practice in the kernel.
I understand that the mif6ctl structure is part of the uAPI
and changing its geometry is not allowed. However, in this
case, we are not changing the geometry of the structure,
as the size of the structure remains the same and the offset
of the structure members will not change. Thus, the proposed
change will not affect the ABI or the user API. Instead, it
will allow the kernel to handle 32-bit ifindex values without
any issues, which is essential for the smooth functioning of
the PIM6 protocol. I hope this explanation clarifies any
concerns you may have had. Let me know if you have any further
questions or need any more details.

Signed-off-by: Ganesh Babu <ganesh.babu@...nops.com>

From: Jakub Kicinski <kuba@...nel.org>
Sent: 29 March 2023 07:44
To: Ganesh Babu <ganesh.babu@...nops.com>
Cc: netdev@...r.kernel.org <netdev@...r.kernel.org>; linux-kernel@...r.kernel.org <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] net: mroute6.h: change type of mif6c_pifi to __u32 

On Tue, 28 Mar 2023 07:13:03 +0000 Ganesh Babu wrote:
> From a91f11fe060729d0009a3271e3a92cead88e2656 Mon Sep 17 00:00:00 2001
> From: "Ganesh Babu" <ganesh.babu@...nops.com>
> Date: Wed, 15 Mar 2023 15:01:39 +0530
> Subject: [PATCH] net: mroute6.h: change type of mif6c_pifi to __u32
> 
> Increase mif6c_pifi field in mif6ctl struct
> from 16 to 32 bits to support 32-bit ifindices.
> The field stores the physical interface (ifindex) for a multicast group.
> Passing a 32-bit ifindex via MRT6_ADD_MIF socket option
> from user space can cause unpredictable behavior in PIM6.
> Changing mif6c_pifi to __u32 allows kernel to handle
> 32-bit ifindex values without issues.

The patch is not formatted correctly.
Maybe try git send-email next time?

> diff --git a/include/uapi/linux/mroute6.h b/include/uapi/linux/mroute6.h
> index 1d90c21a6251..90e6e771beab 100644
> --- a/include/uapi/linux/mroute6.h
> +++ b/include/uapi/linux/mroute6.h
> @@ -75,7 +75,7 @@ struct mif6ctl {
>         mifi_t  mif6c_mifi;             /* Index of MIF */
>         unsigned char mif6c_flags;      /* MIFF_ flags */
>         unsigned char vifc_threshold;   /* ttl limit */
> -       __u16    mif6c_pifi;            /* the index of the physical IF */
> +       __u32    mif6c_pifi;            /* the index of the physical IF */

Unfortunately we can't do this. The structure is part of uAPI,
we can't change it's geometry. The kernel must maintain binary
backward compatibility. 

>         unsigned int vifc_rate_limit;   /* Rate limiter values (NI) */
>  };
> 
> --
> 2.11.0
> 
> Signed-off-by: Ganesh Babu <ganesh.babu@...nops.com>
> ---