Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.38 as permitted sender) client-ip=23.128.96.38;
From:   Anjali Kulkarni <anjali.k.kulkarni@oracle.com>
To:     Simon Horman <horms@kernel.org>
CC:     open list <linux-kernel@vger.kernel.org>,
        "David S. Miller" <davem@davemloft.net>,
        Liam Howlett <liam.howlett@oracle.com>,
        Netdev <netdev@vger.kernel.org>,
        kernel test robot <oliver.sang@intel.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Anjali Kulkarni <anjali.k.kulkarni@oracle.com>
Subject: Re: [PATCH v1] Fix NULL pointer dereference in cn_filter()
Thread-Topic: [PATCH v1] Fix NULL pointer dereference in cn_filter()
Thread-Index: AQHZ/ih9jUSSRZynuUmcpd9TV4mzXbBNpCQAgAW6LgA=
Date:   Fri, 20 Oct 2023 23:30:02 +0000
Message-ID: <6C29584E-AF58-42E0-84A5-DE2F3FE49F16@oracle.com>
References: <20231013225619.987912-1-anjali.k.kulkarni@oracle.com>
 <20231017080210.GG1751252@kernel.org>
In-Reply-To: <20231017080210.GG1751252@kernel.org>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?lKsn1iV1T8zHopNBhuxP4lEh9Kc6pT23utXJULyfyPP1+lismYjiOvStcPQh?=
 =?us-ascii?Q?PSsVRik5Mes3roil8/Lm8ISQbAwIFiFLa0DNTIga3pJ2g/XNFpQktPoBRCFp?=
 =?us-ascii?Q?eYwIcGFXQPXYjaropEKI24UHAQkFPSZa43mE7Yk3nEf1MR4g1CdxG6ndfllC?=
 =?us-ascii?Q?902VWuiaImMi+f4om8XUVggqJOnMDzplpjgoGaNrdWjDApAiT8iy8AoSMJt3?=
 =?us-ascii?Q?Zt8EmRoX7S851x5A6TL2XiAwi03BjBIAPSP8gf/6sHiuFLjUCFS+h+pnMK/5?=
 =?us-ascii?Q?NYV3Bje6ojNX1EuJgmHbxaY1tQLKyuGUs9/ROjWHfTEWNdNscNQN1CIFHJPN?=
 =?us-ascii?Q?9IqR11CptpWCcPBXpXF2MbJcdLGKhOoHM2D08Lb0f2YQz+qkC8ebi+jOGtDo?=
 =?us-ascii?Q?GQN84vknTYrFBecyw5GOPaxxTRr3gKUWMHQPuXWFQr4+qZKXzF9F5/WxDeB+?=
 =?us-ascii?Q?GM/gPbEZUkulSRxcST/pcAVK3gWgXas+SboWZgRhsboVjtOcCz+S/UGxqdv3?=
 =?us-ascii?Q?9JOT4piMnce4jizrHyBL1Yil5kPazCWV5Z4H4CPNw2Vna30kfF5VmFYkhbZY?=
 =?us-ascii?Q?SPNd7cKijfX0XAHFQXFpdfPwlTibd4wl/eGrEOT/O6mvRokrO3vMwOQnGZ0G?=
 =?us-ascii?Q?mfZTJtWDN5l4qFZaKCTUTThvQoF0OmUmmJCn9HUAoPH/mEwc/8tqmKkPW+T5?=
 =?us-ascii?Q?TQdYOBciimA59IKr+nErHMytG8y1/l/kKwWb9dMmEb0Cr9wOlJrp737mQIgy?=
 =?us-ascii?Q?TA6O1GnHcx+o4vtG5ABOnRHPTcES00HQuJgVvh+PU6RMsRiupB2QF9pPREe4?=
 =?us-ascii?Q?dF4AFBVxIrMbA0TcnPheAYCaqokm7oCA/0an8/6O2LUlwiC95762wkW017hI?=
 =?us-ascii?Q?xrBbt0Nefowgwhfg+t607O26QQ66clL2JWhWLLza6TpwsB5uc24f8k+pXAGR?=
 =?us-ascii?Q?FLwlOiaX/EXnJ6wXgZrl0Ro/bjstjEWhWm8LQlWfr8PeLLPCziqV6qfYHVXq?=
 =?us-ascii?Q?FybEFNfyM14sdYyU38II3y7n5p0s37PpH5ha4yzfyqLKrCP7Dks/or3+Z8fk?=
 =?us-ascii?Q?oREH+uBbm/8nxIn93ua4UIgxglHMWg6HnKb19ovxIe1XUjVyaXZXWDhCFFhe?=
 =?us-ascii?Q?mXngtjyZxx104a0KLms0/Jwz3WYufX/9/p7X9tziI7hJUSibV3Alp73Bo+AI?=
 =?us-ascii?Q?eyY8l5xRbmnpFxxonUzoB3eHe53qvT6xk6lVJZJv564Vtzbbwhmpw7r0qxDA?=
 =?us-ascii?Q?f/MljSBBtGz6ntNXdQ76POMazwnWAoYSdNegMtaE9uXNSZoY+J9N2YlUSRXt?=
 =?us-ascii?Q?Q3WRCtmuwm87DVXxpBXrQ5hH0p17sqdg3YuV2AxsUpy84H0aHCro7Kt6TVU3?=
 =?us-ascii?Q?byuruvNpSZjJDwP66WZ5Qpo2ccf9feEZB+Q6A4G44IrzENk7ZgsNXawlzqyQ?=
 =?us-ascii?Q?zd57oVui+fefzCjashQHIULuyA935CXQ8somTmMpibabd5Yl8k0g4dczV8cd?=
 =?us-ascii?Q?t9qK7gxZ1+zSXvMNsBdCWXPjp6ssNiVLuo93kQYODKS499Ir0nNKRus5fl9e?=
 =?us-ascii?Q?e8McA/fx0iafa7Ij+iotO9Pw8le61beOKGi+ZGxAfCqf1e94KN3yoQ2isVOe?=
 =?us-ascii?Q?ZA=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <F2AA941A9EC0D7499A770686118BC100@namprd10.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: k13g98cGnd8FcW7IIouX6tW07BcmauSAe+AjLlbz2kKZzkeaeSsWT81D8wz/6AFbmjrA5KSNSvYjoVUU1Fjc8r0KvUURZRxJfdp4mdEP374DZIVhOHaWy4A77642nSNWMcgME4/c4Cj2U4F24BlmjvyPWW3wtcpnPJMUvpUPnBLCTnGuzGOkZPfWsBHE/B2Nl7LKjcMicTnGrw5cUcrfXohjOZOFGzJdNRBiSxocRdcqqDTE78wkv2+xV0BlcbmJzzZOOvXRGTWqebEPzzn2HCXXQ3rawa6xOFdGcmImcJwv2gQ6miVyv0JedzcKt5GIbi/9ppdITGGsbq0AxJeLb4QhLT/tf4oa54Yj25riFnRJrl/uj6Uqd7n5BN7aFvw3XKNhdH/DTvh/xJ3GkYGMyLcYsX6HSaGEKbO9H7bk3LwfuT8VH4ycZqL2XS6mkC3WxWdUBIkQoauyPxXYpdDKtZdwnNvMhNzx5+w6Z4L4/meAA3DpB1azP7oAjWcckrhU5Bq1r3ol0mbmA8hnCbTp28ui0KOXejhtrQzxDeN3UkjxoIG6b5IHESZWsNQHD8kVQ8NxGanv92KBAcQyW6bpg1/UFax1OQJXz6hFsTrwaN9I4owJRmlteECNxzbc8XvFrVBt+sxwucbEkKPolVMB1LVolKIgrNx7iJN20E9ezC/6xfMbuZoFjoe23icVn3zl5ZkiQsUxxdo1H8hKoyACxXh8b9oLJdhHdkACbCP7MSnHTWBK6e8E7zvfSj3W04+AuwgateQZxF3DNfVteRFhMNG2xObL9ijhYlFaG10hSn3hUlDtludfB+aXnJNs3vCYW80QlKSGD3ye2dQeRWj4PL6aGglAQioaFcTAqZh07s0=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: BY5PR10MB4129.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: bdd3f763-d259-4fbc-d52a-08dbd1c47bee
X-MS-Exchange-CrossTenant-originalarrivaltime: 20 Oct 2023 23:30:02.5958
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: mXgyO0+S/VOPqwkqwUJKmjeT/0LomWIipIe6KzmmCligm+ZtFt/F5A/eUue3nvXc+QGO/c6V5QCt6Hdd0YYc2XTuPbOONTwVDYo0m399YlM=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR10MB6506
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.272,Aquarius:18.0.980,Hydra:6.0.619,FMLib:17.11.176.26
 definitions=2023-10-20_10,2023-10-19_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 phishscore=0 bulkscore=0 malwarescore=0
 mlxscore=0 suspectscore=0 spamscore=0 mlxlogscore=999 adultscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2310170001
 definitions=main-2310200200
X-Proofpoint-GUID: teKnHRHzu0UykFC57-fDA2BQ2vUVn1K9
X-Proofpoint-ORIG-GUID: teKnHRHzu0UykFC57-fDA2BQ2vUVn1K9
X-Spam-Status: No, score=-0.8 required=5.0 tests=DKIM_SIGNED,DKIM_VALID,
	DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,
	SPF_HELO_NONE,SPF_PASS autolearn=unavailable autolearn_force=no
	version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on fry.vger.email
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (fry.vger.email [0.0.0.0]); Fri, 20 Oct 2023 16:30:31 -0700 (PDT)


> On Oct 17, 2023, at 1:02 AM, Simon Horman <horms@kernel.org> wrote:
>=20
> On Fri, Oct 13, 2023 at 03:56:19PM -0700, Anjali Kulkarni wrote:
>> Check that sk_user_data is not NULL, else return from cn_filter().
>=20
> Thanks,
>=20
> I agree that this change seems likely to address the problem at the link
> below. And I also think cn_filter() is a good place to fix this [1].
> But I am wondering if you could add some commentary to the patch
> description, describing under what circumstances this problem can occur.

Hi,

We always allocate sk_user_data in cn_proc_mcast_ctl(), which should ideall=
y
happen before any fork event is triggered, which ends up calling cn_filter(=
),
where we need to use sk_user_data. So in the normal flow of events, sk_user=
_data
should not be NULL in cn_filter(). I also looked at whether there were any =
race
conditions which could cause this issue and found one possibility - if a fo=
rk
event was triggered between the time that the bind() call was made by the
application (which ends up adding the socket to the multicast list for CONN=
ECTOR
maintained by netlink layer), and the call to cn_proc_mcast_ctl(), then the=
 netlink
layer will try to send the event to the sockets in the multicast list via  =
cn_filter() and find that sk_user_data is NULL.
However, when I tried to reproduce this case, I could not do it by adding a=
 wait
between bind() & cn_proc_mcast_ctl(). On looking at the code, I did see tha=
t
in proc_fork_connector(), we do check for proc_event_num_listeners being gr=
eater
than 0 before we invoke netlink layer's send message, which calls cn_filter=
(). And
proc_event_num_listeners is only incremented after sk_user_data is allocate=
d. This
is why we cannot reproduce it this way. Perhaps there is some other way I
am not aware of, but I could not think of anything else.
I will just resend the patch.

Thanks
Anjali

>=20
> [1] https://urldefense.com/v3/__https://lore.kernel.org/all/2023101312010=
5.GH29570@kernel.org/__;!!ACWV5N9M2RV99hQ!NNsMHaho3lyW2NyoT8Sju1BL78S5pNu5A=
htZC76cc1Xb1_psXwfP0lmVtVmTxGRrsQkzClWNS8HriosTMols$=20
>=20
>> Fixes: 2aa1f7a1f47c ("connector/cn_proc: Add filtering to fix some bugs"=
)
>> Reported-by: kernel test robot <oliver.sang@intel.com>
>> Closes: https://urldefense.com/v3/__https://lore.kernel.org/oe-lkp/20230=
9201456.84c19e27-oliver.sang@intel.com/__;!!ACWV5N9M2RV99hQ!NNsMHaho3lyW2Ny=
oT8Sju1BL78S5pNu5AhtZC76cc1Xb1_psXwfP0lmVtVmTxGRrsQkzClWNS8HrirVfNms7$=20
>> Signed-off-by: Anjali Kulkarni <anjali.k.kulkarni@oracle.com>
>> ---
>> drivers/connector/cn_proc.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>=20
>> diff --git a/drivers/connector/cn_proc.c b/drivers/connector/cn_proc.c
>> index 05d562e9c8b1..a8e55569e4f5 100644
>> --- a/drivers/connector/cn_proc.c
>> +++ b/drivers/connector/cn_proc.c
>> @@ -54,7 +54,7 @@ static int cn_filter(struct sock *dsk, struct sk_buff =
*skb, void *data)
>> enum proc_cn_mcast_op mc_op;
>> uintptr_t val;
>>=20
>> - if (!dsk || !data)
>> + if (!dsk || !data || !dsk->sk_user_data)
>> return 0;
>>=20
>> ptr =3D (__u32 *)data;
>> --=20
>> 2.42.0
>>=20