[PATCH 1/2 net] ptp: fix error codes in ptp_clock_register()

The ptp_clock_register() function returns NULL when it's #ifdefed out
because CONFIG_PTP_1588_CLOCK is disabled.  Otherwise, it's intended to
return error pointers.  Unfortunately, there are a couple paths where we
forget to set the error code.  It means that we could result in NULL
pointer dereferences in the callers.

Fixes: d94ba80ebbea ("ptp: Added a brand new class driver for ptp clocks.")
Signed-off-by: Dan Carpenter <redacted>

diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c
index b77435783ef3..a50e96143a25 100644
--- a/drivers/ptp/ptp_clock.c
+++ b/drivers/ptp/ptp_clock.c

@@ -190,7 +190,7 @@ struct ptp_clock *ptp_clock_register(struct ptp_clock_info *info,
 				     struct device *parent)
 {
 	struct ptp_clock *ptp;
-	int err = 0, index, major = MAJOR(ptp_devt);
+	int err, index, major = MAJOR(ptp_devt);
 
 	if (info->n_alarm > PTP_MAX_ALARMS)
 		return ERR_PTR(-EINVAL);

@@ -225,8 +225,10 @@ struct ptp_clock *ptp_clock_register(struct ptp_clock_info *info,
 	ptp->dev = device_create_with_groups(ptp_class, parent, ptp->devid,
 					     ptp, ptp->pin_attr_groups,
 					     "ptp%d", ptp->index);
-	if (IS_ERR(ptp->dev))
+	if (IS_ERR(ptp->dev)) {
+		err = PTR_ERR(ptp->dev);
 		goto no_device;
+	}
 
 	/* Register a new PPS source. */
 	if (info->pps) {

@@ -238,6 +240,7 @@ struct ptp_clock *ptp_clock_register(struct ptp_clock_info *info,
 		ptp->pps_source = pps_register_source(&pps, PTP_PPS_DEFAULTS);
 		if (!ptp->pps_source) {
 			pr_err("failed to register pps source\n");
+			err = -ENOMEM;
 			goto no_pps;
 		}
 	}

We're checking ptp_clock_register() for NULL but we should be checking
for error pointers.

Fixes: 9c33e4208bce ("cxgb4: Add PTP Hardware Clock (PHC) support")
Signed-off-by: Dan Carpenter <redacted>

diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_ptp.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_ptp.c
index 50517cfd9671..c24313a103c6 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_ptp.c
+++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_ptp.c

@@ -441,7 +441,7 @@ void cxgb4_ptp_init(struct adapter *adapter)
 
 	adapter->ptp_clock = ptp_clock_register(&adapter->ptp_clock_info,
 						&adapter->pdev->dev);
-	if (!adapter->ptp_clock) {
+	if (IS_ERR(adapter->ptp_clock)) {
 		dev_err(adapter->pdev_dev,
 			"PTP %s Clock registration has failed\n", __func__);
 		return;

On Mon, Jul 10, 2017 at 10:11:37AM +0300, Dan Carpenter wrote:

The ptp_clock_register() function returns NULL when it's #ifdefed out
because CONFIG_PTP_1588_CLOCK is disabled.  Otherwise, it's intended to
return error pointers.  Unfortunately, there are a couple paths where we
forget to set the error code.  It means that we could result in NULL
pointer dereferences in the callers.

Fixes: d94ba80ebbea ("ptp: Added a brand new class driver for ptp clocks.")

This "Fixes" tag references the wrong commit.  Please correct it.

Thanks,
Richard

On Mon, Jul 10, 2017 at 10:11:37AM +0300, Dan Carpenter wrote:

The ptp_clock_register() function returns NULL when it's #ifdefed out
because CONFIG_PTP_1588_CLOCK is disabled.  Otherwise, it's intended to
return error pointers.  Unfortunately, there are a couple paths where we
forget to set the error code.  It means that we could result in NULL
pointer dereferences in the callers.

Actually, this description is bogus.  Callers will not dereference
NULL, because they are required to check the returned pointer:

/**
 * ptp_clock_register() - register a PTP hardware clock driver
 *
 * @info:   Structure describing the new clock.
 * @parent: Pointer to the parent device of the new clock.
 *
 * Returns a valid pointer on success or PTR_ERR on failure.  If PHC
 * support is missing at the configuration level, this function
 * returns NULL, and drivers are expected to gracefully handle that
 * case separately.
 */

Thanks,
Richard

On Mon, Jul 10, 2017 at 11:21:03AM +0200, Richard Cochran wrote:

On Mon, Jul 10, 2017 at 10:11:37AM +0300, Dan Carpenter wrote:

quoted

The ptp_clock_register() function returns NULL when it's #ifdefed out
because CONFIG_PTP_1588_CLOCK is disabled.  Otherwise, it's intended to
return error pointers.  Unfortunately, there are a couple paths where we
forget to set the error code.  It means that we could result in NULL
pointer dereferences in the callers.

Fixes: d94ba80ebbea ("ptp: Added a brand new class driver for ptp clocks.")

This "Fixes" tag references the wrong commit.  Please correct it.

There were two buggy commits so I chose the ealier one.  The other buggy
commit is 85a66e550195 ("ptp: create "pins" together with the rest of
attributes").  I should have CC'd Dmitry as well.  I can resend.

regards,
dan carpenter

On Mon, Jul 10, 2017 at 12:38:16PM +0300, Dan Carpenter wrote:

There were two buggy commits so I chose the ealier one.  The other buggy

No, you are mistaken.  In the original patch, NULL or PTR_ERR were
returned on error, and that was not a bug.

If you want to correct the present version of ptp_clock_register() to
always return a valid pointer or PTR_ERR (like the kerneldoc says), be
my guest, but please say that in the change log and reference the
correct commit (namely the one related to disabling POSIX clocks.)

Thanks,
Richard

On Mon, Jul 10, 2017 at 10:16:15AM +0300, Dan Carpenter wrote:

We're checking ptp_clock_register() for NULL but we should be checking
for error pointers.

No.

Yeah, that is wrong, but the fix is to check to IS_ERR or NULL.

Thanks,
Richard

On Mon, Jul 10, 2017 at 11:48:06AM +0200, Richard Cochran wrote:

On Mon, Jul 10, 2017 at 12:38:16PM +0300, Dan Carpenter wrote:

quoted

There were two buggy commits so I chose the ealier one.  The other buggy

No, you are mistaken.  In the original patch, NULL or PTR_ERR were
returned on error, and that was not a bug.

The "goto no_pps" was a bug you introduced.

But I feel like you're being rude, so I'm not going to resend these
patches.  Please fix them yourself.

regards,
dan carpenter

On Mon, Jul 10, 2017 at 01:29:20PM +0300, Dan Carpenter wrote:

The "goto no_pps" was a bug you introduced.

I took a second look, and yes, the original commit should not have
returned NULL, and the original callers did not expect NULL either.
 

But I feel like you're being rude, so I'm not going to resend these
patches.  Please fix them yourself.

Sorry you feel that way.  Your first patch, although now more or less
cosmetic, would be okay.  The second patch is still wrong.

Thanks,
Richard