diff mbox

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

Message ID 20170710071137.ui2fhjkqzjqhkopz@mwanda
State Changes Requested, archived
Delegated to: David Miller
Headers show

Commit Message

Dan Carpenter July 10, 2017, 7:11 a.m. UTC 
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 <dan.carpenter@oracle.com>

Comments

Richard Cochran July 10, 2017, 9:21 a.m. UTC | #1 
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
Richard Cochran July 10, 2017, 9:34 a.m. UTC | #2 
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
Dan Carpenter July 10, 2017, 9:38 a.m. UTC | #3 
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:
> > 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
Richard Cochran July 10, 2017, 9:48 a.m. UTC | #4 
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
Dan Carpenter July 10, 2017, 10:29 a.m. UTC | #5 
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:
> > 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
Richard Cochran July 10, 2017, 11:02 a.m. UTC | #6 
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
diff mbox

Patch

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;
 		}
 	}