| Message ID | 20231212024015.11595-1-chentao@kylinos.cn (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v2,iwl-next] ice: Fix some null pointer dereference issues in ice_ptp.c | expand |
On 12/12/23 03:40, 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> > Suggested-by: Przemek Kitszel <przemyslaw.kitszel@intel.com> You found the bug (or some some static analysis tool in that case); there is no need to add Suggested-by for every person that suggests something during review - the tag is for "person/s that suggested making such change in the repo". Subject line would be better if less generic, eg: ice: avoid null deref of ptp auxbus name > Signed-off-by: Kunwu Chan <chentao@kylinos.cn> > --- > drivers/net/ethernet/intel/ice/ice_ptp.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c > index e9e59f4b5580..848e3e063e64 100644 > --- a/drivers/net/ethernet/intel/ice/ice_ptp.c > +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c > @@ -2743,6 +2743,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; > @@ -2989,6 +2991,8 @@ 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) > + return -ENOMEM; > > aux_dev->name = name; > aux_dev->id = id; Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com> Regarding iwl-next vs iwl-net: this bug is really unlikely to manifest, as we take care of both earlier and future mem allocs for ptp auxbus, and auxiliary_device_init() checks for null name, so no big deal, so: -next is fine
On Wed, Dec 13, 2023 at 10:49:10AM +0100, Przemek Kitszel wrote: > On 12/12/23 03:40, 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> > > Suggested-by: Przemek Kitszel <przemyslaw.kitszel@intel.com> > > You found the bug (or some some static analysis tool in that case); > there is no need to add Suggested-by for every person that suggests > something during review - the tag is for "person/s that suggested > making such change in the repo". > > Subject line would be better if less generic, eg: > ice: avoid null deref of ptp auxbus name > > > Signed-off-by: Kunwu Chan <chentao@kylinos.cn> > > --- > > drivers/net/ethernet/intel/ice/ice_ptp.c | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c > > index e9e59f4b5580..848e3e063e64 100644 > > --- a/drivers/net/ethernet/intel/ice/ice_ptp.c > > +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c > > @@ -2743,6 +2743,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; > > @@ -2989,6 +2991,8 @@ 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) > > + return -ENOMEM; > > aux_dev->name = name; > > aux_dev->id = id; > > Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com> > > Regarding iwl-next vs iwl-net: this bug is really unlikely to manifest, > as we take care of both earlier and future mem allocs for ptp auxbus, > and auxiliary_device_init() checks for null name, so no big deal, > so: -next is fine Thanks. FWIIW, this looks good to me too. Reviewed-by: Simon Horman <horms@kernel.org>
> -----Original Message----- > From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of Kunwu Chan > Sent: Tuesday, December 12, 2023 8:10 AM > To: Brandeburg, Jesse <jesse.brandeburg@intel.com>; Nguyen, Anthony L <anthony.l.nguyen@intel.com>; davem@davemloft.net; edumazet@google.com; kuba@kernel.org; pabeni@redhat.com; richardcochran@gmail.com; Keller, Jacob E <jacob.e.keller@intel.com> > Cc: Michalik, Michal <michal.michalik@intel.com>; Kunwu Chan <chentao@kylinos.cn>; Kunwu Chan <kunwu.chan@hotmail.com>; netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Kolacinski, Karol <karol.kolacinski@intel.com>; intel-wired-lan@lists.osuosl.org; Kitszel, Przemyslaw <przemyslaw.kitszel@intel.com> > Subject: [Intel-wired-lan] [PATCH v2 iwl-next] ice: Fix some null pointer dereference issues in ice_ptp.c > > 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> > Suggested-by: Przemek Kitszel <przemyslaw.kitszel@intel.com> > Signed-off-by: Kunwu Chan <chentao@kylinos.cn> > --- > drivers/net/ethernet/intel/ice/ice_ptp.c | 4 ++++ > 1 file changed, 4 insertions(+) > Tested-by: Pucha Himasekhar Reddy <himasekharx.reddy.pucha@intel.com> (A Contingent worker at Intel)
diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c index e9e59f4b5580..848e3e063e64 100644 --- a/drivers/net/ethernet/intel/ice/ice_ptp.c +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c @@ -2743,6 +2743,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; @@ -2989,6 +2991,8 @@ 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) + return -ENOMEM; aux_dev->name = name; aux_dev->id = id;
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> Suggested-by: Przemek Kitszel <przemyslaw.kitszel@intel.com> Signed-off-by: Kunwu Chan <chentao@kylinos.cn> --- drivers/net/ethernet/intel/ice/ice_ptp.c | 4 ++++ 1 file changed, 4 insertions(+)