[PATCH net-next 0/3] ptp: feature enhancements

[PATCH net-next 0/3] ptp: feature enhancements

[Richard Cochran]

This series adds one driver specific enhancement and one new feature
to the PTP Hardware Clock subsystem. The first patch enables more of
the phyter's IO capabilities. The second patch introduces PTP one-step
support for Sync messages at the driver level. The third patch
implements the one-step flag in the phyter.

Richard Cochran (3):
  dp83640: enable six external events and one periodic output
  net: introduce ptp one step time stamp mode for sync packets
  dp83640: add time stamp insertion for sync messages

 drivers/net/phy/dp83640.c  |  205 +++++++++++++++++++++++++++++++++++++-------
 include/linux/net_tstamp.h |    9 ++
 2 files changed, 182 insertions(+), 32 deletions(-)

-- 
1.7.2.5

[PATCH net-next 1/3] dp83640: enable six external events and one periodic output

[Richard Cochran]

This patch enables six external event channels and one periodic output.
One GPIO is reserved for synchronizing multiple PHYs. The assignment
of GPIO functions can be changed via a module parameter.

The code supports multiple simultaneous events by inducing a PTP clock
event for every channel marked in the PHY's extended status word.

Signed-off-by: Richard Cochran <richard.cochran@omicron.at>
---
 drivers/net/phy/dp83640.c |  135 ++++++++++++++++++++++++++++++++++++++------
 1 files changed, 116 insertions(+), 19 deletions(-)

diff --git a/drivers/net/phy/dp83640.c b/drivers/net/phy/dp83640.c
index edd7304..d3c6a2e 100644
--- a/drivers/net/phy/dp83640.c
+++ b/drivers/net/phy/dp83640.c
@@ -35,16 +35,15 @@
 #define LAYER4		0x02
 #define LAYER2		0x01
 #define MAX_RXTS	64
-#define N_EXT_TS	1
+#define N_EXT_TS	6
 #define PSF_PTPVER	2
 #define PSF_EVNT	0x4000
 #define PSF_RX		0x2000
 #define PSF_TX		0x1000
 #define EXT_EVENT	1
-#define EXT_GPIO	1
-#define CAL_EVENT	2
-#define CAL_GPIO	9
-#define CAL_TRIGGER	2
+#define CAL_EVENT	7
+#define CAL_TRIGGER	7
+#define PER_TRIGGER	6
 
 /* phyter seems to miss the mark by 16 ns */
 #define ADJTIME_FIX	16
@@ -131,16 +130,30 @@ struct dp83640_clock {
 
 /* globals */
 
+enum {
+	CALIBRATE_GPIO,
+	PEROUT_GPIO,
+	EXTTS0_GPIO,
+	EXTTS1_GPIO,
+	EXTTS2_GPIO,
+	EXTTS3_GPIO,
+	EXTTS4_GPIO,
+	EXTTS5_GPIO,
+	GPIO_TABLE_SIZE
+};
+
 static int chosen_phy = -1;
-static ushort cal_gpio = 4;
+static ushort gpio_tab[GPIO_TABLE_SIZE] = {
+	1, 2, 3, 4, 8, 9, 10, 11
+};
 
 module_param(chosen_phy, int, 0444);
-module_param(cal_gpio, ushort, 0444);
+module_param_array(gpio_tab, ushort, NULL, 0444);
 
 MODULE_PARM_DESC(chosen_phy, \
 	"The address of the PHY to use for the ancillary clock features");
-MODULE_PARM_DESC(cal_gpio, \
-	"Which GPIO line to use for synchronizing multiple PHYs");
+MODULE_PARM_DESC(gpio_tab, \
+	"Which GPIO line to use for which purpose: cal,perout,extts1,...,extts6");
 
 /* a list of clocks and a mutex to protect it */
 static LIST_HEAD(phyter_clocks);
@@ -235,6 +248,61 @@ static u64 phy2txts(struct phy_txts *p)
 	return ns;
 }
 
+static void periodic_output(struct dp83640_clock *clock,
+			    struct ptp_clock_request *clkreq, bool on)
+{
+	struct dp83640_private *dp83640 = clock->chosen;
+	struct phy_device *phydev = dp83640->phydev;
+	u32 sec, nsec, period;
+	u16 gpio, ptp_trig, trigger, val;
+
+	gpio = on ? gpio_tab[PEROUT_GPIO] : 0;
+	trigger = PER_TRIGGER;
+
+	ptp_trig = TRIG_WR |
+		(trigger & TRIG_CSEL_MASK) << TRIG_CSEL_SHIFT |
+		(gpio & TRIG_GPIO_MASK) << TRIG_GPIO_SHIFT |
+		TRIG_PER |
+		TRIG_PULSE;
+
+	val = (trigger & TRIG_SEL_MASK) << TRIG_SEL_SHIFT;
+
+	if (!on) {
+		val |= TRIG_DIS;
+		mutex_lock(&clock->extreg_lock);
+		ext_write(0, phydev, PAGE5, PTP_TRIG, ptp_trig);
+		ext_write(0, phydev, PAGE4, PTP_CTL, val);
+		mutex_unlock(&clock->extreg_lock);
+		return;
+	}
+
+	sec = clkreq->perout.start.sec;
+	nsec = clkreq->perout.start.nsec;
+	period = clkreq->perout.period.sec * 1000000000UL;
+	period += clkreq->perout.period.nsec;
+
+	mutex_lock(&clock->extreg_lock);
+
+	ext_write(0, phydev, PAGE5, PTP_TRIG, ptp_trig);
+
+	/*load trigger*/
+	val |= TRIG_LOAD;
+	ext_write(0, phydev, PAGE4, PTP_CTL, val);
+	ext_write(0, phydev, PAGE4, PTP_TDR, nsec & 0xffff);   /* ns[15:0] */
+	ext_write(0, phydev, PAGE4, PTP_TDR, nsec >> 16);      /* ns[31:16] */
+	ext_write(0, phydev, PAGE4, PTP_TDR, sec & 0xffff);    /* sec[15:0] */
+	ext_write(0, phydev, PAGE4, PTP_TDR, sec >> 16);       /* sec[31:16] */
+	ext_write(0, phydev, PAGE4, PTP_TDR, period & 0xffff); /* ns[15:0] */
+	ext_write(0, phydev, PAGE4, PTP_TDR, period >> 16);    /* ns[31:16] */
+
+	/*enable trigger*/
+	val &= ~TRIG_LOAD;
+	val |= TRIG_EN;
+	ext_write(0, phydev, PAGE4, PTP_CTL, val);
+
+	mutex_unlock(&clock->extreg_lock);
+}
+
 /* ptp clock methods */
 
 static int ptp_dp83640_adjfreq(struct ptp_clock_info *ptp, s32 ppb)
@@ -338,19 +406,30 @@ static int ptp_dp83640_enable(struct ptp_clock_info *ptp,
 	struct dp83640_clock *clock =
 		container_of(ptp, struct dp83640_clock, caps);
 	struct phy_device *phydev = clock->chosen->phydev;
-	u16 evnt;
+	int index;
+	u16 evnt, event_num, gpio_num;
 
 	switch (rq->type) {
 	case PTP_CLK_REQ_EXTTS:
-		if (rq->extts.index != 0)
+		index = rq->extts.index;
+		if (index < 0 || index >= N_EXT_TS)
 			return -EINVAL;
-		evnt = EVNT_WR | (EXT_EVENT & EVNT_SEL_MASK) << EVNT_SEL_SHIFT;
+		event_num = EXT_EVENT + index;
+		evnt = EVNT_WR | (event_num & EVNT_SEL_MASK) << EVNT_SEL_SHIFT;
 		if (on) {
-			evnt |= (EXT_GPIO & EVNT_GPIO_MASK) << EVNT_GPIO_SHIFT;
+			gpio_num = gpio_tab[EXTTS0_GPIO + index];
+			evnt |= (gpio_num & EVNT_GPIO_MASK) << EVNT_GPIO_SHIFT;
 			evnt |= EVNT_RISE;
 		}
 		ext_write(0, phydev, PAGE5, PTP_EVNT, evnt);
 		return 0;
+
+	case PTP_CLK_REQ_PEROUT:
+		if (rq->perout.index != 0)
+			return -EINVAL;
+		periodic_output(clock, rq, on);
+		return 0;
+
 	default:
 		break;
 	}
@@ -441,9 +520,10 @@ static void recalibrate(struct dp83640_clock *clock)
 	struct list_head *this;
 	struct dp83640_private *tmp;
 	struct phy_device *master = clock->chosen->phydev;
-	u16 cfg0, evnt, ptp_trig, trigger, val;
+	u16 cal_gpio, cfg0, evnt, ptp_trig, trigger, val;
 
 	trigger = CAL_TRIGGER;
+	cal_gpio = gpio_tab[CALIBRATE_GPIO];
 
 	mutex_lock(&clock->extreg_lock);
 
@@ -542,11 +622,17 @@ static void recalibrate(struct dp83640_clock *clock)
 
 /* time stamping methods */
 
+static inline u16 exts_chan_to_edata(int ch)
+{
+	return 1 << ((ch + EXT_EVENT) * 2);
+}
+
 static int decode_evnt(struct dp83640_private *dp83640,
 		       void *data, u16 ests)
 {
 	struct phy_txts *phy_txts;
 	struct ptp_clock_event event;
+	int i, parsed;
 	int words = (ests >> EVNT_TS_LEN_SHIFT) & EVNT_TS_LEN_MASK;
 	u16 ext_status = 0;
 
@@ -568,14 +654,25 @@ static int decode_evnt(struct dp83640_private *dp83640,
 		dp83640->edata.ns_lo = phy_txts->ns_lo;
 	}
 
+	if (ext_status) {
+		parsed = words + 2;
+	} else {
+		parsed = words + 1;
+		i = ((ests >> EVNT_NUM_SHIFT) & EVNT_NUM_MASK) - EXT_EVENT;
+		ext_status = exts_chan_to_edata(i);
+	}
+
 	event.type = PTP_CLOCK_EXTTS;
-	event.index = 0;
 	event.timestamp = phy2txts(&dp83640->edata);
 
-	ptp_clock_event(dp83640->clock->ptp_clock, &event);
+	for (i = 0; i < N_EXT_TS; i++) {
+		if (ext_status & exts_chan_to_edata(i)) {
+			event.index = i;
+			ptp_clock_event(dp83640->clock->ptp_clock, &event);
+		}
+	}
 
-	words = ext_status ? words + 2 : words + 1;
-	return words * sizeof(u16);
+	return parsed * sizeof(u16);
 }
 
 static void decode_rxts(struct dp83640_private *dp83640,
@@ -740,7 +837,7 @@ static void dp83640_clock_init(struct dp83640_clock *clock, struct mii_bus *bus)
 	clock->caps.max_adj	= 1953124;
 	clock->caps.n_alarm	= 0;
 	clock->caps.n_ext_ts	= N_EXT_TS;
-	clock->caps.n_per_out	= 0;
+	clock->caps.n_per_out	= 1;
 	clock->caps.pps		= 0;
 	clock->caps.adjfreq	= ptp_dp83640_adjfreq;
 	clock->caps.adjtime	= ptp_dp83640_adjtime;
-- 
1.7.2.5

[PATCH net-next 2/3] net: introduce ptp one step time stamp mode for sync packets

[Richard Cochran]

The IEEE 1588 standard (PTP) has a provision for a "one step" mode, where
time stamps on outgoing event packets are inserted into the packet by the
hardware on the fly. This patch adds a new flag for the SIOCSHWTSTAMP
ioctl that lets user space programs request this mode.

Signed-off-by: Richard Cochran <richard.cochran@omicron.at>
---
 include/linux/net_tstamp.h |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/include/linux/net_tstamp.h b/include/linux/net_tstamp.h
index a3b8546..3df0984 100644
--- a/include/linux/net_tstamp.h
+++ b/include/linux/net_tstamp.h
@@ -60,6 +60,15 @@ enum {
 	 * before sending the packet.
 	 */
 	HWTSTAMP_TX_ON,
+
+	/*
+	 * Enables time stamping for outgoing packets just as
+	 * HWTSTAMP_TX_ON does, but also enables time stamp insertion
+	 * directly into Sync packets. In this case, transmitted Sync
+	 * packets will not received a time stamp via the socket error
+	 * queue.
+	 */
+	HWTSTAMP_TX_ONESTEP_SYNC,
 };
 
 /* possible values for hwtstamp_config->rx_filter */
-- 
1.7.2.5

[PATCH net-next 3/3] dp83640: add time stamp insertion for sync messages

[Richard Cochran]

This commit adds one step support to the phyter. When enabled, the
hardware does not provide time stamps for transmitted sync messages but
instead inserts the stamp into the outgoing packet.

Signed-off-by: Richard Cochran <richard.cochran@omicron.at>
---
 drivers/net/phy/dp83640.c |   70 ++++++++++++++++++++++++++++++++++++--------
 1 files changed, 57 insertions(+), 13 deletions(-)

diff --git a/drivers/net/phy/dp83640.c b/drivers/net/phy/dp83640.c
index d3c6a2e..c588a16 100644
--- a/drivers/net/phy/dp83640.c
+++ b/drivers/net/phy/dp83640.c
@@ -761,6 +761,41 @@ static void decode_status_frame(struct dp83640_private *dp83640,
 	}
 }
 
+static int is_sync(struct sk_buff *skb, int type)
+{
+	u8 *data = skb->data, *msgtype;
+	unsigned int offset = 0;
+
+	switch (type) {
+	case PTP_CLASS_V1_IPV4:
+	case PTP_CLASS_V2_IPV4:
+		offset = ETH_HLEN + IPV4_HLEN(data) + UDP_HLEN;
+		break;
+	case PTP_CLASS_V1_IPV6:
+	case PTP_CLASS_V2_IPV6:
+		offset = OFF_PTP6;
+		break;
+	case PTP_CLASS_V2_L2:
+		offset = ETH_HLEN;
+		break;
+	case PTP_CLASS_V2_VLAN:
+		offset = ETH_HLEN + VLAN_HLEN;
+		break;
+	default:
+		return 0;
+	}
+
+	if (type & PTP_CLASS_V1)
+		offset += OFF_PTP_CONTROL;
+
+	if (skb->len < offset + 1)
+		return 0;
+
+	msgtype = data + offset;
+
+	return (*msgtype & 0xf) == 0;
+}
+
 static int match(struct sk_buff *skb, unsigned int type, struct rxts *rxts)
 {
 	u16 *seqid;
@@ -1010,16 +1045,10 @@ static int dp83640_hwtstamp(struct phy_device *phydev, struct ifreq *ifr)
 	if (cfg.flags) /* reserved for future extensions */
 		return -EINVAL;
 
-	switch (cfg.tx_type) {
-	case HWTSTAMP_TX_OFF:
-		dp83640->hwts_tx_en = 0;
-		break;
-	case HWTSTAMP_TX_ON:
-		dp83640->hwts_tx_en = 1;
-		break;
-	default:
+	if (cfg.tx_type < 0 || cfg.tx_type > HWTSTAMP_TX_ONESTEP_SYNC)
 		return -ERANGE;
-	}
+
+	dp83640->hwts_tx_en = cfg.tx_type;
 
 	switch (cfg.rx_filter) {
 	case HWTSTAMP_FILTER_NONE:
@@ -1074,6 +1103,9 @@ static int dp83640_hwtstamp(struct phy_device *phydev, struct ifreq *ifr)
 	if (dp83640->hwts_tx_en)
 		txcfg0 |= TX_TS_EN;
 
+	if (dp83640->hwts_tx_en == HWTSTAMP_TX_ONESTEP_SYNC)
+		txcfg0 |= SYNC_1STEP | CHK_1STEP;
+
 	if (dp83640->hwts_rx_en)
 		rxcfg0 |= RX_TS_EN;
 
@@ -1156,12 +1188,24 @@ static void dp83640_txtstamp(struct phy_device *phydev,
 {
 	struct dp83640_private *dp83640 = phydev->priv;
 
-	if (!dp83640->hwts_tx_en) {
+	switch (dp83640->hwts_tx_en) {
+
+	case HWTSTAMP_TX_ONESTEP_SYNC:
+		if (is_sync(skb, type)) {
+			kfree_skb(skb);
+			return;
+		}
+		/* fall through */
+	case HWTSTAMP_TX_ON:
+		skb_queue_tail(&dp83640->tx_queue, skb);
+		schedule_work(&dp83640->ts_work);
+		break;
+
+	case HWTSTAMP_TX_OFF:
+	default:
 		kfree_skb(skb);
-		return;
+		break;
 	}
-	skb_queue_tail(&dp83640->tx_queue, skb);
-	schedule_work(&dp83640->ts_work);
 }
 
 static struct phy_driver dp83640_driver = {
-- 
1.7.2.5

Re: [PATCH net-next 0/3] ptp: feature enhancements

[David Miller]

From: Richard Cochran <richardcochran@gmail.com>
Date: Tue, 20 Sep 2011 13:43:13 +0200

> This series adds one driver specific enhancement and one new feature
> to the PTP Hardware Clock subsystem. The first patch enables more of
> the phyter's IO capabilities. The second patch introduces PTP one-step
> support for Sync messages at the driver level. The third patch
> implements the one-step flag in the phyter.
> 
> Richard Cochran (3):
>   dp83640: enable six external events and one periodic output
>   net: introduce ptp one step time stamp mode for sync packets
>   dp83640: add time stamp insertion for sync messages

All applied to net-next, thanks.

Could you please add a test at the point where we pull in the
->tx_type value from the user, to make sure it is one of the
validly defined HWTSTAMP_TX_FOO values?

Thanks.

Re: [PATCH net-next 0/3] ptp: feature enhancements

[Richard Cochran]

On Mon, Sep 26, 2011 at 04:04:43PM -0400, David Miller wrote:
> Could you please add a test at the point where we pull in the
> ->tx_type value from the user, to make sure it is one of the
> validly defined HWTSTAMP_TX_FOO values?

Okay, will do. Any chance of getting the bug fixes in for 3.1?

Thanks,
Richard

Re: [PATCH net-next 0/3] ptp: feature enhancements

[David Miller]

From: Richard Cochran <richardcochran@gmail.com>
Date: Wed, 28 Sep 2011 10:00:24 +0200

> On Mon, Sep 26, 2011 at 04:04:43PM -0400, David Miller wrote:
>> Could you please add a test at the point where we pull in the
>> ->tx_type value from the user, to make sure it is one of the
>> validly defined HWTSTAMP_TX_FOO values?
> 
> Okay, will do. Any chance of getting the bug fixes in for 3.1?

I plan to try to get them into the next round.

[PATCH net-next 0/1] net: validate HWTSTAMP ioctl parameters

[Richard Cochran]

Okay, here is a patch to check the ioctl.

I took the liberty to check both the Rx and Tx fields. I coded the
check rather pedantically, using a switch/case over the enumerated
values. The advantage of this style is that the compiler will warn
if someone adds a new enum into net_tstamp.h without fixing the
checking function in dev.c.

Richard Cochran (1):
  net: validate HWTSTAMP ioctl parameters

 include/linux/net_tstamp.h |    4 +-
 net/core/dev.c             |   58 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 60 insertions(+), 2 deletions(-)

-- 
1.7.2.5

[PATCH net-next 1/1] net: validate HWTSTAMP ioctl parameters

[Richard Cochran]

This patch adds a sanity check on the values provided by user space for
the hardware time stamping configuration. If the values lie outside of
the absolute limits, then the ioctl request will be denied.

Signed-off-by: Richard Cochran <richard.cochran@omicron.at>
---
 include/linux/net_tstamp.h |    4 +-
 net/core/dev.c             |   58 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 60 insertions(+), 2 deletions(-)

diff --git a/include/linux/net_tstamp.h b/include/linux/net_tstamp.h
index 3df0984..ae5df12 100644
--- a/include/linux/net_tstamp.h
+++ b/include/linux/net_tstamp.h
@@ -45,7 +45,7 @@ struct hwtstamp_config {
 };
 
 /* possible values for hwtstamp_config->tx_type */
-enum {
+enum hwtstamp_tx_types {
 	/*
 	 * No outgoing packet will need hardware time stamping;
 	 * should a packet arrive which asks for it, no hardware
@@ -72,7 +72,7 @@ enum {
 };
 
 /* possible values for hwtstamp_config->rx_filter */
-enum {
+enum hwtstamp_rx_filters {
 	/* time stamp no incoming packet at all */
 	HWTSTAMP_FILTER_NONE,
 
diff --git a/net/core/dev.c b/net/core/dev.c
index 70ecb86..bc347e6 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -136,6 +136,7 @@
 #include <linux/if_tunnel.h>
 #include <linux/if_pppox.h>
 #include <linux/ppp_defs.h>
+#include <linux/net_tstamp.h>
 
 #include "net-sysfs.h"
 
@@ -1477,6 +1478,57 @@ static inline void net_timestamp_check(struct sk_buff *skb)
 		__net_timestamp(skb);
 }
 
+static int net_hwtstamp_validate(struct ifreq *ifr)
+{
+	struct hwtstamp_config cfg;
+	enum hwtstamp_tx_types tx_type;
+	enum hwtstamp_rx_filters rx_filter;
+	int tx_type_valid = 0;
+	int rx_filter_valid = 0;
+
+	if (copy_from_user(&cfg, ifr->ifr_data, sizeof(cfg)))
+		return -EFAULT;
+
+	if (cfg.flags) /* reserved for future extensions */
+		return -EINVAL;
+
+	tx_type = cfg.tx_type;
+	rx_filter = cfg.rx_filter;
+
+	switch (tx_type) {
+	case HWTSTAMP_TX_OFF:
+	case HWTSTAMP_TX_ON:
+	case HWTSTAMP_TX_ONESTEP_SYNC:
+		tx_type_valid = 1;
+		break;
+	}
+
+	switch (rx_filter) {
+	case HWTSTAMP_FILTER_NONE:
+	case HWTSTAMP_FILTER_ALL:
+	case HWTSTAMP_FILTER_SOME:
+	case HWTSTAMP_FILTER_PTP_V1_L4_EVENT:
+	case HWTSTAMP_FILTER_PTP_V1_L4_SYNC:
+	case HWTSTAMP_FILTER_PTP_V1_L4_DELAY_REQ:
+	case HWTSTAMP_FILTER_PTP_V2_L4_EVENT:
+	case HWTSTAMP_FILTER_PTP_V2_L4_SYNC:
+	case HWTSTAMP_FILTER_PTP_V2_L4_DELAY_REQ:
+	case HWTSTAMP_FILTER_PTP_V2_L2_EVENT:
+	case HWTSTAMP_FILTER_PTP_V2_L2_SYNC:
+	case HWTSTAMP_FILTER_PTP_V2_L2_DELAY_REQ:
+	case HWTSTAMP_FILTER_PTP_V2_EVENT:
+	case HWTSTAMP_FILTER_PTP_V2_SYNC:
+	case HWTSTAMP_FILTER_PTP_V2_DELAY_REQ:
+		rx_filter_valid = 1;
+		break;
+	}
+
+	if (!tx_type_valid || !rx_filter_valid)
+		return -ERANGE;
+
+	return 0;
+}
+
 static inline bool is_skb_forwardable(struct net_device *dev,
 				      struct sk_buff *skb)
 {
@@ -4921,6 +4973,12 @@ static int dev_ifsioc(struct net *net, struct ifreq *ifr, unsigned int cmd)
 		ifr->ifr_newname[IFNAMSIZ-1] = '\0';
 		return dev_change_name(dev, ifr->ifr_newname);
 
+	case SIOCSHWTSTAMP:
+		err = net_hwtstamp_validate(ifr);
+		if (err)
+			return err;
+		/* fall through */
+
 	/*
 	 *	Unknown or private ioctl
 	 */
-- 
1.7.2.5

Re: [PATCH net-next 1/1] net: validate HWTSTAMP ioctl parameters

[David Miller]

From: Richard Cochran <richardcochran@gmail.com>
Date: Fri, 14 Oct 2011 11:37:48 +0200

> This patch adds a sanity check on the values provided by user space for
> the hardware time stamping configuration. If the values lie outside of
> the absolute limits, then the ioctl request will be denied.
> 
> Signed-off-by: Richard Cochran <richard.cochran@omicron.at>

Thanks a lot for following up on this.

Applied, thanks again Richard.

Re: [PATCH net-next 1/1] net: validate HWTSTAMP ioctl parameters

[Ben Hutchings]

On Fri, 2011-10-14 at 11:37 +0200, Richard Cochran wrote:
> This patch adds a sanity check on the values provided by user space for
> the hardware time stamping configuration. If the values lie outside of
> the absolute limits, then the ioctl request will be denied.
[...]

What does this validation buy us?  The driver still has to copy the
values into kernel space again, at which point they may have been
changed to be invalid.  Depending on how the driver uses them (perhaps
as array indices), it may have to validate them again to avoid a
security vulnerability.

I think that either SIOCHWTSTAMP should be handled through a discrete
device operation (not ndo_ioctl) which receives a pointer to the
validated structure in kernel memory, or a validation function should be
exported to drivers so that they can call it from their ndo_ioctl
implementations after copying the structure into kernel memory.

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH net-next 1/1] net: validate HWTSTAMP ioctl parameters

[Richard Cochran]

On Wed, Oct 19, 2011 at 10:16:56PM +0100, Ben Hutchings wrote:
> On Fri, 2011-10-14 at 11:37 +0200, Richard Cochran wrote:
> > This patch adds a sanity check on the values provided by user space for
> > the hardware time stamping configuration. If the values lie outside of
> > the absolute limits, then the ioctl request will be denied.
> [...]
> 
> What does this validation buy us?  The driver still has to copy the
> values into kernel space again, at which point they may have been
> changed to be invalid.  Depending on how the driver uses them (perhaps
> as array indices), it may have to validate them again to avoid a
> security vulnerability.

Oops, you are right.

The drivers will really need to check the configuration again in any
case, since no driver will support every option.

I understood David's request as simply a sanity check on the absolute
limits.

> I think that either SIOCHWTSTAMP should be handled through a discrete
> device operation (not ndo_ioctl) which receives a pointer to the
> validated structure in kernel memory,

Okay, I'll take a stab at this.

> or a validation function should be
> exported to drivers so that they can call it from their ndo_ioctl
> implementations after copying the structure into kernel memory.

I think it better to do the sanity check in one place, to guard
against lazy or sloppy drivers.

Thanks,
Richard