Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) client-ip=2620:137:e000::3:7;
Message-ID: <447d130a-e1ac-02f6-e168-b04bfa9c6004@intel.com>
Date:   Mon, 11 Dec 2023 08:15:53 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.3.1
Subject: Re: [PATCH] ice: Fix some null pointer dereference issues in
 ice_ptp.c
Content-Language: en-US
To:     Kunwu Chan <chentao@kylinos.cn>, <jesse.brandeburg@intel.com>,
        <anthony.l.nguyen@intel.com>, <davem@davemloft.net>,
        <edumazet@google.com>, <kuba@kernel.org>, <pabeni@redhat.com>,
        <richardcochran@gmail.com>, <jacob.e.keller@intel.com>,
        <karol.kolacinski@intel.com>
CC:     <michal.michalik@intel.com>, <intel-wired-lan@lists.osuosl.org>,
        <netdev@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        Kunwu Chan <kunwu.chan@hotmail.com>
References: <20231211062649.247148-1-chentao@kylinos.cn>
From:   Przemek Kitszel <przemyslaw.kitszel@intel.com>
In-Reply-To: <20231211062649.247148-1-chentao@kylinos.cn>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?VmJuWnRWdnRhNmRUSXVmenVyaUhmeS9DalNFWnFHeVB1V05sYWtKSHhTTjBO?=
 =?utf-8?B?YUxpbkhMR1QrQnNiVG1LenF6djBlejg1QVVvVklYKzRMKzNyUllEaHFudy9G?=
 =?utf-8?B?ZlhjMkIrbTJmT2JQdmVBTmR0ZVVYV0IxRFg5cHEvbCtSa2FmSjJZTzU0dmhi?=
 =?utf-8?B?NUh1OWo3bHFUVlQxUS9ld1U4bXdBazVJVVhhYTdtcDc2eGE5bVJWVGphQUVT?=
 =?utf-8?B?d2h0YzQ3Ni9Ga29lekx4dDNHV1E0STVJQ2tJVm5sY0lsSmxCSWFLZTJ5ZEtx?=
 =?utf-8?B?YVp2QmhMeWZOd3BOVXZES2dlSHVYajJSd0JLTzhKVDEyODhrWVhvUjE3b01N?=
 =?utf-8?B?R2VyaHRlWSs0WUkwSHhVUi94RWF3MGs4SFE2bTlGbW5MT3B5Z3lXV2dDcTdk?=
 =?utf-8?B?VHlvbXpjS1BwODBLOXRydEhZaG5yL291SEpTYlpKdDlvanBKTTF6ZTNuRUV6?=
 =?utf-8?B?YWVYNnBDWEtyTW5TTHhZTW0wUksxM0d2RlpmTzFwQVFxb3V4ZDFpTHBzYlRN?=
 =?utf-8?B?MkJSbUZIYUFCZW13YzhmWkVnK3Zpb2NpdHQxRHFLODVjb3lkUnIvUFVuU3hF?=
 =?utf-8?B?Q3AxdE1zSXNxV21xN1dKVDdtZFo0YUt0QS9BT2JFbDM3VmpHRmovcUFKYVIv?=
 =?utf-8?B?TXJ3Vnk0WkltR2Iyd3dYMVhLTW9xWEdCc3M0MkVQcWJDODFZdDExTFZkSkI2?=
 =?utf-8?B?eHRpelU2M2IwZGFVNUFPS0FlazhZOWVMMlJRK2tNS3V3bE9qbGNaVHRrZUFq?=
 =?utf-8?B?bEh2anFtWWtpTy9KY2hNdFdrSVNSd05HZWFOMmZNbUlpTEhpOWtVcmZnNXNJ?=
 =?utf-8?B?OWg4SUdzR2hBQTZIMDk2SVgxajkyNktPRnNGTlRJK2h6ZDA2M2lIQ0xaNlBT?=
 =?utf-8?B?ajU0ZW1yZEJMOHNnSjJKcnZFWEtjKzVxVWQwNmdCc3RBV1dvYU4rZVR0UkFK?=
 =?utf-8?B?OFdOQkZ1SEQybC82U0V2SW9IRDluR201aGw2V3VsM0FrR1kxb0g4eXlXWnZP?=
 =?utf-8?B?TlJUeHczWHlxOHRpSWdPa21Uckh0NGgvTi9JanVra0hrNUV4NzNUTzFKeEJC?=
 =?utf-8?B?QXpWZW5sYmJWTTE4OEROZUZscUFzUXIwZmVVc2s1WW5rMFZIOVAzbEtId3hM?=
 =?utf-8?B?RHZ1eFRXV1lyclcvSlFJM0ZPN0gvRnhQODgxOEJ0ZFR3L205M2JBbDZ5cWpa?=
 =?utf-8?B?NVBMQ2JPQ01Ya21zTGxqaHNmV1RZenhRZVVteDFRWDZzTkZ0MkFCNWNCVEtj?=
 =?utf-8?B?SVF1Lzdpb3Fucy9sWmNIcHpBTlhzSGFIelBBU2VLTlZtd1E0RVE5VXp3Y3py?=
 =?utf-8?B?TDlJWVovR1F5bTRWS01qQkNkVjdmZ2k4Y2tiOVBlSW1UMytMeXMvRXc2cFBF?=
 =?utf-8?B?aElDbkk2ekNHbFlBTzBCRkJkS2NZZXkxYUFyenVTZjdjVW51VXdYeTF2MTdn?=
 =?utf-8?B?ek1qTmFmYjBwOU0yU2VNZi9TcVZ1aWUybzVmYm01ODdPaDRyaVpGc2oyV3cw?=
 =?utf-8?B?VmswWmNkWWIvYUhzQmpyanVTTWRlODZ6bWtPWFJkdkhUSTZVZ0lQNXVQU0ln?=
 =?utf-8?B?cXV6SDhpaW5UOGY4QVNNTXRHbXFMbEFpUjJ2ZTNQK21QaUk1T3hBV3N4anY0?=
 =?utf-8?B?RktNcGJoZlB3TGJMRUlLeTcvdVZDTG1MdHJ4b241QnJBdGVseTlZeU1obko1?=
 =?utf-8?B?YW5OdWE0YXMzd2ZzZ0p5NExCV09XUHRrU2VKbHNSb3VsZlZ0MC94cUhvR1hx?=
 =?utf-8?B?VHZuaTJscmpoWW4xQmkyUUhhbzNWdjcrSnpHLzFPSmV1MTdFZ3VnUDJqdmU3?=
 =?utf-8?B?U1BGWmNOc1FpcHhMNUk0WDU4Q1NNRForUUFxTWRKRjJvZkZ3T1czZFB0VE5T?=
 =?utf-8?B?NDFXS3h3d0JTK2xMOVJIZGFEU2dpMDgvRkJKK25DaTNLaVVzUnd6dDRMY3g0?=
 =?utf-8?B?dFNuNUxralYraUpmb3FsMzV2bDBVWG85cHI4dWhOdW9qektIS21NYS9oK0pI?=
 =?utf-8?B?SVB5WW5RUVdOeDRRSHprUnFQMkMxTHd2bXFFZU4zTE9LakJ5M2ZwTHF4S1RV?=
 =?utf-8?B?V3RlZ0k1SkVXQXlCVm1xd0swOWRKbUlqNmpXKzd4NFJMSm9Bd3ZIZEx2dE0v?=
 =?utf-8?B?cjdFNGpWQjc5SWRwdFE3ek5QbVdFbUZ5blBpMzFpUDEzWS9lUXdKQnFsSEtl?=
 =?utf-8?B?QWc9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 05a468e1-a888-45a5-d729-08dbfa1908b5
X-MS-Exchange-CrossTenant-AuthSource: BYAPR11MB3672.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 11 Dec 2023 07:16:03.2876
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: IUKcL4vcUEIKvnQJU3/JYNWOPeGaCrJhHnr8MIzGvhC5oERxJaVdcxjMl/xovb/W6A02kz5bSy7B079Upe1/QSSBx06mjIPIsmwHlV0ztrc=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY5PR11MB6137
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-5.0 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,
        RCVD_IN_DNSWL_BLOCKED,SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE
        autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
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 (snail.vger.email [0.0.0.0]); Sun, 10 Dec 2023 23:16:38 -0800 (PST)

On 12/11/23 07:26, Kunwu Chan wrote:
> devm_kasprintf() returns a pointer to dynamically allocated memory
> which can be NULL upon failure.
> 
> Fixes: d938a8cca88a ("ice: Auxbus devices & driver for E822 TS")
> Cc: Kunwu Chan <kunwu.chan@hotmail.com>
> Signed-off-by: Kunwu Chan <chentao@kylinos.cn>

I would suggest adding "iwl-net" as a target here

> ---
>   drivers/net/ethernet/intel/ice/ice_ptp.c | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
> index 1eddcbe89b0c..59794ce4f243 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
> @@ -2668,6 +2668,8 @@ static int ice_ptp_register_auxbus_driver(struct ice_pf *pf)
>   	name = devm_kasprintf(dev, GFP_KERNEL, "ptp_aux_dev_%u_%u_clk%u",
>   			      pf->pdev->bus->number, PCI_SLOT(pf->pdev->devfn),
>   			      ice_get_ptp_src_clock_index(&pf->hw));
> +	if (!name)
> +		return -ENOMEM;
>   
>   	aux_driver->name = name;
>   	aux_driver->shutdown = ice_ptp_auxbus_shutdown;
> @@ -2929,6 +2931,10 @@ static int ice_ptp_create_auxbus_device(struct ice_pf *pf)
>   	name = devm_kasprintf(dev, GFP_KERNEL, "ptp_aux_dev_%u_%u_clk%u",
>   			      pf->pdev->bus->number, PCI_SLOT(pf->pdev->devfn),
>   			      ice_get_ptp_src_clock_index(&pf->hw));
> +	if (!name) {
> +		dev_err(dev, "Failed to allocate memory\n");

Kuba @ [1]:
no warnings on allocation failures, there will be a splat for GFP_KERNEL
(checkpatch should catch this)

[1] https://lore.kernel.org/netdev/20231206195304.6226771d@kernel.org/T/

so just "return -ENOMEM" would be sufficient

> +		return -ENOMEM;
> +	}
>   
>   	aux_dev->name = name;
>   	aux_dev->id = id;

I didn't checked but having same code in two places raises questions.
Are you overwriting old name here, or our code is just self similar?