[PATCH 0/5] firewire: cdev interface updates

[PATCH 0/5] firewire: cdev interface updates

[Stefan Richter]

I'll follow up with these proposed updates to the character device file
interface of firewire-core:

 firewire: cdev: amendment to "add ioctl to query maximum transmission speed"
 firewire: cdev: secure add_descriptor ioctl
 firewire: cdev: fix race of ioctl_send_request with bus reset
 firewire: cdev: simplify FW_CDEV_IOC_SEND_REQUEST return value
 firewire: cdev: add closure to async stream ioctl

 drivers/firewire/fw-cdev.c        |   79 ++++++++++++++----------------
 drivers/firewire/fw-transaction.c |   42 ++++++---------
 drivers/firewire/fw-transaction.h |    9 +--
 include/linux/firewire-cdev.h     |   73 ++++++++++++---------------
 4 files changed, 93 insertions(+), 110 deletions(-)

Please comment.
-- 
Stefan Richter
-=====-==--= --== -=-=-
http://arcgraph.de/sr/

[PATCH 1/5] firewire: cdev: amendment to "add ioctl to query maximum transmission speed"

[Stefan Richter]

The as yet unreleased FW_CDEV_IOC_GET_SPEED ioctl puts only a single
integer into the parameter buffer.  We can use ioctl()'s return value
instead.

(Also: Some whitespace change in firewire-cdev.h.)

Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
---
 drivers/firewire/fw-cdev.c    |   11 +++++-----
 include/linux/firewire-cdev.h |   37 ++++++++++++----------------------
 2 files changed, 20 insertions(+), 28 deletions(-)

Index: linux/drivers/firewire/fw-cdev.c
===================================================================
--- linux.orig/drivers/firewire/fw-cdev.c
+++ linux/drivers/firewire/fw-cdev.c
@@ -1214,13 +1214,14 @@ static int ioctl_deallocate_iso_resource
 	return init_iso_resource(client, request, ISO_RES_DEALLOC_ONCE);
 }
 
+/*
+ * Returns a speed code:  Maximum speed to or from this device,
+ * limited by the device's link speed, the local node's link speed,
+ * and all PHY port speeds between the two links.
+ */
 static int ioctl_get_speed(struct client *client, void *buffer)
 {
-	struct fw_cdev_get_speed *request = buffer;
-
-	request->max_speed = client->device->max_speed;
-
-	return 0;
+	return client->device->max_speed;
 }
 
 static int ioctl_send_broadcast_request(struct client *client, void *buffer)
Index: linux/include/linux/firewire-cdev.h
===================================================================
--- linux.orig/include/linux/firewire-cdev.h
+++ linux/include/linux/firewire-cdev.h
@@ -223,28 +223,28 @@ union fw_cdev_event {
 };
 
 /* available since kernel version 2.6.22 */
-#define FW_CDEV_IOC_GET_INFO		_IOWR('#', 0x00, struct fw_cdev_get_info)
-#define FW_CDEV_IOC_SEND_REQUEST	_IOW('#', 0x01, struct fw_cdev_send_request)
-#define FW_CDEV_IOC_ALLOCATE		_IOWR('#', 0x02, struct fw_cdev_allocate)
-#define FW_CDEV_IOC_DEALLOCATE		_IOW('#', 0x03, struct fw_cdev_deallocate)
-#define FW_CDEV_IOC_SEND_RESPONSE	_IOW('#', 0x04, struct fw_cdev_send_response)
-#define FW_CDEV_IOC_INITIATE_BUS_RESET	_IOW('#', 0x05, struct fw_cdev_initiate_bus_reset)
-#define FW_CDEV_IOC_ADD_DESCRIPTOR	_IOWR('#', 0x06, struct fw_cdev_add_descriptor)
-#define FW_CDEV_IOC_REMOVE_DESCRIPTOR	_IOW('#', 0x07, struct fw_cdev_remove_descriptor)
-#define FW_CDEV_IOC_CREATE_ISO_CONTEXT	_IOWR('#', 0x08, struct fw_cdev_create_iso_context)
-#define FW_CDEV_IOC_QUEUE_ISO		_IOWR('#', 0x09, struct fw_cdev_queue_iso)
-#define FW_CDEV_IOC_START_ISO		_IOW('#', 0x0a, struct fw_cdev_start_iso)
-#define FW_CDEV_IOC_STOP_ISO		_IOW('#', 0x0b, struct fw_cdev_stop_iso)
+#define FW_CDEV_IOC_GET_INFO           _IOWR('#', 0x00, struct fw_cdev_get_info)
+#define FW_CDEV_IOC_SEND_REQUEST        _IOW('#', 0x01, struct fw_cdev_send_request)
+#define FW_CDEV_IOC_ALLOCATE           _IOWR('#', 0x02, struct fw_cdev_allocate)
+#define FW_CDEV_IOC_DEALLOCATE          _IOW('#', 0x03, struct fw_cdev_deallocate)
+#define FW_CDEV_IOC_SEND_RESPONSE       _IOW('#', 0x04, struct fw_cdev_send_response)
+#define FW_CDEV_IOC_INITIATE_BUS_RESET  _IOW('#', 0x05, struct fw_cdev_initiate_bus_reset)
+#define FW_CDEV_IOC_ADD_DESCRIPTOR     _IOWR('#', 0x06, struct fw_cdev_add_descriptor)
+#define FW_CDEV_IOC_REMOVE_DESCRIPTOR   _IOW('#', 0x07, struct fw_cdev_remove_descriptor)
+#define FW_CDEV_IOC_CREATE_ISO_CONTEXT _IOWR('#', 0x08, struct fw_cdev_create_iso_context)
+#define FW_CDEV_IOC_QUEUE_ISO          _IOWR('#', 0x09, struct fw_cdev_queue_iso)
+#define FW_CDEV_IOC_START_ISO           _IOW('#', 0x0a, struct fw_cdev_start_iso)
+#define FW_CDEV_IOC_STOP_ISO            _IOW('#', 0x0b, struct fw_cdev_stop_iso)
 
 /* available since kernel version 2.6.24 */
-#define FW_CDEV_IOC_GET_CYCLE_TIMER	_IOR('#', 0x0c, struct fw_cdev_get_cycle_timer)
+#define FW_CDEV_IOC_GET_CYCLE_TIMER     _IOR('#', 0x0c, struct fw_cdev_get_cycle_timer)
 
 /* available since kernel version 2.6.30 */
 #define FW_CDEV_IOC_ALLOCATE_ISO_RESOURCE       _IOWR('#', 0x0d, struct fw_cdev_allocate_iso_resource)
 #define FW_CDEV_IOC_DEALLOCATE_ISO_RESOURCE      _IOW('#', 0x0e, struct fw_cdev_deallocate)
 #define FW_CDEV_IOC_ALLOCATE_ISO_RESOURCE_ONCE   _IOW('#', 0x0f, struct fw_cdev_allocate_iso_resource)
 #define FW_CDEV_IOC_DEALLOCATE_ISO_RESOURCE_ONCE _IOW('#', 0x10, struct fw_cdev_allocate_iso_resource)
-#define FW_CDEV_IOC_GET_SPEED                    _IOR('#', 0x11, struct fw_cdev_get_speed)
+#define FW_CDEV_IOC_GET_SPEED                     _IO('#', 0x11) /* returns speed code */
 #define FW_CDEV_IOC_SEND_BROADCAST_REQUEST       _IOW('#', 0x12, struct fw_cdev_send_request)
 #define FW_CDEV_IOC_SEND_STREAM_PACKET           _IOW('#', 0x13, struct fw_cdev_send_stream_packet)
 
@@ -602,15 +602,6 @@ struct fw_cdev_allocate_iso_resource {
 };
 
 /**
- * struct fw_cdev_get_speed - Query maximum speed to or from this device
- * @max_speed:	Speed code; minimum of the device's link speed, the local node's
- *		link speed, and all PHY port speeds between the two links
- */
-struct fw_cdev_get_speed {
-	__u32 max_speed;
-};
-
-/**
  * struct fw_cdev_send_stream_packet - send an asynchronous stream packet
  * @generation:   Bus generation where the packet is valid
  * @speed:	  Speed code to send the packet at

-- 
Stefan Richter
-=====-==--= --== -=-=-
http://arcgraph.de/sr/

[PATCH 2/5] firewire: cdev: secure add_descriptor ioctl

[Stefan Richter]

The access permissions and ownership or ACL of /dev/fw* character device
files will typically be set based on the device type of the respective
nodes, as obtained by firewire-core from descriptors in the device's
configuration ROM.  An example policy is to deny write permission by
default but grant write permission to files of AV/C video and audio
devices and IIDC video devices.

The FW_CDEV_IOC_ADD_DESCRIPTOR ioctl could be used to partly subvert
such a policy:  Find a device file with relaxed permissions, use the
ioctl to add a descriptor with AV/C marker to the local node's ROM, thus
gain access to the local node's character device file.  (This is only
possible if there are udev scripts installed which actively relax
permissions for known device types and if there is a device of such a
type connected.)

Accessibility of the local node's device file is relevant to host
security if the host contains two or more IEEE 1394 link layer
controllers which are plugged into a single bus.

Therefore change the ABI to deny FW_CDEV_IOC_ADD_DESCRIPTOR if the file
belongs to a remote node.  (This change has no impact on known
implementers of the ABI:  None of them uses the ioctl yet.)

Also clarify the documentation:  The ioctl affects all local nodes, not
just one local node.

Cc: stable@kernel.org
Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
---
 drivers/firewire/fw-cdev.c    |    8 ++++++++
 include/linux/firewire-cdev.h |    5 ++++-
 2 files changed, 12 insertions(+), 1 deletion(-)

Index: linux/drivers/firewire/fw-cdev.c
===================================================================
--- linux.orig/drivers/firewire/fw-cdev.c
+++ linux/drivers/firewire/fw-cdev.c
@@ -742,9 +742,17 @@ static void release_descriptor(struct cl
 static int ioctl_add_descriptor(struct client *client, void *buffer)
 {
 	struct fw_cdev_add_descriptor *request = buffer;
+	struct fw_card *card = client->device->card;
 	struct descriptor_resource *r;
 	int ret;
 
+	/* Access policy: Allow this ioctl only on local nodes' device files. */
+	spin_lock_irq(&card->lock);
+	ret = client->device->node_id != card->local_node->node_id;
+	spin_unlock_irq(&card->lock);
+	if (ret)
+		return -ENOSYS;
+
 	if (request->length > 256)
 		return -EINVAL;
 
Index: linux/include/linux/firewire-cdev.h
===================================================================
--- linux.orig/include/linux/firewire-cdev.h
+++ linux/include/linux/firewire-cdev.h
@@ -394,6 +394,9 @@ struct fw_cdev_initiate_bus_reset {
  * If successful, the kernel adds the descriptor and writes back a handle to the
  * kernel-side object to be used for later removal of the descriptor block and
  * immediate key.
+ *
+ * This ioctl affects the configuration ROMs of all local nodes.
+ * The ioctl only succeeds on device files which represent a local node.
  */
 struct fw_cdev_add_descriptor {
 	__u32 immediate;
@@ -409,7 +412,7 @@ struct fw_cdev_add_descriptor {
  *		descriptor was added
  *
  * Remove a descriptor block and accompanying immediate key from the local
- * node's configuration ROM.
+ * nodes' configuration ROMs.
  */
 struct fw_cdev_remove_descriptor {
 	__u32 handle;

-- 
Stefan Richter
-=====-==--= --== -=-=-
http://arcgraph.de/sr/

[PATCH 3/5] firewire: cdev: fix race of ioctl_send_request with bus reset

[Stefan Richter]

The bus reset handler concurrently frees client->device->node.  Use
device->node_id instead.  This is equivalent to device->node->node_id
while device->generation is current.

Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
---
 drivers/firewire/fw-cdev.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: linux/drivers/firewire/fw-cdev.c
===================================================================
--- linux.orig/drivers/firewire/fw-cdev.c
+++ linux/drivers/firewire/fw-cdev.c
@@ -585,7 +585,7 @@ static int ioctl_send_request(struct cli
 		return -EINVAL;
 	}
 
-	return init_request(client, request, client->device->node->node_id,
+	return init_request(client, request, client->device->node_id,
 			    client->device->max_speed);
 }
 

-- 
Stefan Richter
-=====-==--= --== -=-=-
http://arcgraph.de/sr/

[PATCH 4/5] firewire: cdev: simplify FW_CDEV_IOC_SEND_REQUEST return value

[Stefan Richter]

This changes the ioctl() return value of FW_CDEV_IOC_SEND_REQUEST and of
the as yet unreleased FW_CDEV_IOC_SEND_BROADCAST_REQUEST.  They used to
return
	sizeof(struct fw_cdev_send_request *) + data_length

which is obviously a failed attempt to emulate the return value of
raw1394's respective interface which uses write() instead of ioctl().

However, the first summand, as size of a kernel pointer, is entirely
meaningless to clients and the second summand is already known to
clients.  And the result does not resemble raw1394's write() return
code anyway.

So simplify it to a constant non-negative value, i.e. 0.  The only
dangers here would be that future client implementations check for error
by ret != 0 instead of ret < 0 when running on top of an old kernel; or
that current clients interpret ret = 0 or more as failure.  But both are
hypothetical cases which don't justify to return irritating values.

While we touch this code, also remove "& 0x1f" from tcode in the call of
fw_send_request.  The tcode cannot be bigger than 0x1f at this point.

Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
---
 drivers/firewire/fw-cdev.c |   12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

Index: linux/drivers/firewire/fw-cdev.c
===================================================================
--- linux.orig/drivers/firewire/fw-cdev.c
+++ linux/drivers/firewire/fw-cdev.c
@@ -549,15 +549,11 @@ static int init_request(struct client *c
 	client_get(client);
 
 	fw_send_request(client->device->card, &e->r.transaction,
-			request->tcode & 0x1f, destination_id,
-			request->generation, speed, request->offset,
-			e->response.data, request->length,
-			complete_transaction, e);
+			request->tcode, destination_id, request->generation,
+			speed, request->offset, e->response.data,
+			request->length, complete_transaction, e);
+	return 0;
 
-	if (request->data)
-		return sizeof(request) + request->length;
-	else
-		return sizeof(request);
  failed:
 	kfree(e);
 

-- 
Stefan Richter
-=====-==--= --== -=-=-
http://arcgraph.de/sr/

[PATCH 5/5] firewire: cdev: add closure to async stream ioctl

[Stefan Richter]

This changes the as yet unreleased FW_CDEV_IOC_SEND_STREAM_PACKET ioctl
to generate an fw_cdev_event_response event just like the other two
ioctls for asynchronous request transmission do.  This way, clients get
feedback on successful or unsuccessful transmission.

This also adds input validation for length, tag, channel, sy, speed.

Furthermore, this changes how future in-kernel drivers can transmit
asynchronous streams:
  - The still unused fw_send_stream_packet() is deleted.
  - Instead, call either fw_send_request() as asynchronous API in any
    context or fw_run_transaction() as synchronous API in non-atomic
    context.
  - Supply tag/ channel/ sy via fw_stream_packet_destination_id().
  - In case of fw_send_request(), supply an fw_transaction and an
    fw_transaction_callback_t instead of an fw_packet and an
    fw_packet_callback_t.

Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
---
 drivers/firewire/fw-cdev.c        |   46 ++++++++++++------------------
 drivers/firewire/fw-transaction.c |   42 ++++++++++-----------------
 drivers/firewire/fw-transaction.h |    9 +++--
 include/linux/firewire-cdev.h     |   31 ++++++++++----------
 4 files changed, 56 insertions(+), 72 deletions(-)

Index: linux/drivers/firewire/fw-cdev.c
===================================================================
--- linux.orig/drivers/firewire/fw-cdev.c
+++ linux/drivers/firewire/fw-cdev.c
@@ -522,7 +522,8 @@ static int init_request(struct client *c
 	struct outbound_transaction_event *e;
 	int ret;
 
-	if (request->length > 4096 || request->length > 512 << speed)
+	if (request->tcode != TCODE_STREAM_DATA &&
+	    (request->length > 4096 || request->length > 512 << speed))
 		return -EIO;
 
 	e = kmalloc(sizeof(*e) + request->length, GFP_KERNEL);
@@ -1247,36 +1248,27 @@ static int ioctl_send_broadcast_request(
 	return init_request(client, request, LOCAL_BUS | 0x3f, SCODE_100);
 }
 
-struct stream_packet {
-	struct fw_packet packet;
-	u8 data[0];
-};
-
-static void send_stream_packet_done(struct fw_packet *packet,
-				    struct fw_card *card, int status)
-{
-	kfree(container_of(packet, struct stream_packet, packet));
-}
-
 static int ioctl_send_stream_packet(struct client *client, void *buffer)
 {
-	struct fw_cdev_send_stream_packet *request = buffer;
-	struct stream_packet *p;
+	struct fw_cdev_send_stream_packet *p = buffer;
+	struct fw_cdev_send_request request;
+	int dest;
 
-	p = kmalloc(sizeof(*p) + request->size, GFP_KERNEL);
-	if (p == NULL)
-		return -ENOMEM;
+	if (p->speed > client->device->card->link_speed ||
+	    p->length > 1024 << p->speed)
+		return -EIO;
 
-	if (request->data &&
-	    copy_from_user(p->data, u64_to_uptr(request->data), request->size)) {
-		kfree(p);
-		return -EFAULT;
-	}
-	fw_send_stream_packet(client->device->card, &p->packet,
-			      request->generation, request->speed,
-			      request->channel, request->sy, request->tag,
-			      p->data, request->size, send_stream_packet_done);
-	return 0;
+	if (p->tag > 3 || p->channel > 63 || p->sy > 15)
+		return -EINVAL;
+
+	dest = fw_stream_packet_destination_id(p->tag, p->channel, p->sy);
+	request.tcode		= TCODE_STREAM_DATA;
+	request.length		= p->length;
+	request.closure		= p->closure;
+	request.data		= p->data;
+	request.generation	= p->generation;
+
+	return init_request(client, &request, dest, p->speed);
 }
 
 static int (* const ioctl_handlers[])(struct client *client, void *buffer) = {
Index: linux/drivers/firewire/fw-transaction.c
===================================================================
--- linux.orig/drivers/firewire/fw-transaction.c
+++ linux/drivers/firewire/fw-transaction.c
@@ -37,10 +37,6 @@
 #include "fw-topology.h"
 #include "fw-device.h"
 
-#define HEADER_TAG(tag)			((tag) << 14)
-#define HEADER_CHANNEL(ch)		((ch) << 8)
-#define HEADER_SY(sy)			((sy) << 0)
-
 #define HEADER_PRI(pri)			((pri) << 0)
 #define HEADER_TCODE(tcode)		((tcode) << 4)
 #define HEADER_RETRY(retry)		((retry) << 8)
@@ -158,6 +154,18 @@ static void fw_fill_request(struct fw_pa
 {
 	int ext_tcode;
 
+	if (tcode == TCODE_STREAM_DATA) {
+		packet->header[0] =
+			HEADER_DATA_LENGTH(length) |
+			destination_id |
+			HEADER_TCODE(TCODE_STREAM_DATA);
+		packet->header_length = 4;
+		packet->payload = payload;
+		packet->payload_length = length;
+
+		goto common;
+	}
+
 	if (tcode > 0x10) {
 		ext_tcode = tcode & ~0x10;
 		tcode = TCODE_LOCK_REQUEST;
@@ -204,7 +212,7 @@ static void fw_fill_request(struct fw_pa
 		packet->payload_length = 0;
 		break;
 	}
-
+ common:
 	packet->speed = speed;
 	packet->generation = generation;
 	packet->ack = 0;
@@ -246,6 +254,9 @@ static void fw_fill_request(struct fw_pa
  * @param callback function to be called when the transaction is completed
  * @param callback_data pointer to arbitrary data, which will be
  *   passed to the callback
+ *
+ * In case of asynchronous stream packets i.e. TCODE_STREAM_DATA, the caller
+ * needs to synthesize @destination_id with fw_stream_packet_destination_id().
  */
 void fw_send_request(struct fw_card *card, struct fw_transaction *t, int tcode,
 		     int destination_id, int generation, int speed,
@@ -297,27 +308,6 @@ void fw_send_request(struct fw_card *car
 }
 EXPORT_SYMBOL(fw_send_request);
 
-void fw_send_stream_packet(struct fw_card *card, struct fw_packet *p,
-		int generation, int speed, int channel, int sy, int tag,
-		void *payload, size_t length, fw_packet_callback_t callback)
-{
-	p->callback = callback;
-	p->header[0] =
-		  HEADER_DATA_LENGTH(length)
-		| HEADER_TAG(tag)
-		| HEADER_CHANNEL(channel)
-		| HEADER_TCODE(TCODE_STREAM_DATA)
-		| HEADER_SY(sy);
-	p->header_length = 4;
-	p->payload = payload;
-	p->payload_length = length;
-	p->speed = speed;
-	p->generation = generation;
-	p->ack = 0;
-
-	card->driver->send_request(card, p);
-}
-
 struct transaction_callback_data {
 	struct completion done;
 	void *payload;
Index: linux/drivers/firewire/fw-transaction.h
===================================================================
--- linux.orig/drivers/firewire/fw-transaction.h
+++ linux/drivers/firewire/fw-transaction.h
@@ -412,10 +412,6 @@ void fw_send_request(struct fw_card *car
 		int tcode, int destination_id, int generation, int speed,
 		unsigned long long offset, void *payload, size_t length,
 		fw_transaction_callback_t callback, void *callback_data);
-void fw_send_stream_packet(struct fw_card *card, struct fw_packet *p,
-		int generation, int speed, int channel, int sy, int tag,
-		void *payload, size_t length, fw_packet_callback_t callback);
-
 int fw_cancel_transaction(struct fw_card *card,
 			  struct fw_transaction *transaction);
 void fw_flush_transactions(struct fw_card *card);
@@ -425,6 +421,11 @@ int fw_run_transaction(struct fw_card *c
 void fw_send_phy_config(struct fw_card *card,
 			int node_id, int generation, int gap_count);
 
+static inline int fw_stream_packet_destination_id(int tag, int channel, int sy)
+{
+	return tag << 14 | channel << 8 | sy;
+}
+
 /*
  * Called by the topology code to inform the device code of node
  * activity; found, lost, or updated nodes.
Index: linux/include/linux/firewire-cdev.h
===================================================================
--- linux.orig/include/linux/firewire-cdev.h
+++ linux/include/linux/firewire-cdev.h
@@ -606,28 +606,29 @@ struct fw_cdev_allocate_iso_resource {
 
 /**
  * struct fw_cdev_send_stream_packet - send an asynchronous stream packet
- * @generation:   Bus generation where the packet is valid
- * @speed:	  Speed code to send the packet at
- * @channel:	  Channel to send the packet on
- * @sy:		  Four-bit sy code for the packet
- * @tag:	  Two-bit tag field to use for the packet
- * @size:	  Size of the packet's data payload
- * @data:	  Userspace pointer to the payload
+ * @length:	Length of outgoing payload, in bytes
+ * @tag:	Data format tag
+ * @channel:	Isochronous channel to transmit to
+ * @sy:		Synchronization code
+ * @closure:	Passed back to userspace in the response event
+ * @data:	Userspace pointer to payload
+ * @generation:	The bus generation where packet is valid
+ * @speed:	Speed to transmit at
  *
  * The %FW_CDEV_IOC_SEND_STREAM_PACKET ioctl sends an asynchronous stream packet
- * to every device (that is listening to the specified channel) on the
- * firewire bus.  It is the applications's job to ensure
- * that the intended device(s) will be able to receive the packet at the chosen
- * transmit speed.
+ * to every device which is listening to the specified channel.  The kernel
+ * writes an &fw_cdev_event_response event which indicates success or failure of
+ * the transmission.
  */
 struct fw_cdev_send_stream_packet {
-	__u32 generation;
-	__u32 speed;
+	__u32 length;
+	__u32 tag;
 	__u32 channel;
 	__u32 sy;
-	__u32 tag;
-	__u32 size;
+	__u64 closure;
 	__u64 data;
+	__u32 generation;
+	__u32 speed;
 };
 
 #endif /* _LINUX_FIREWIRE_CDEV_H */

-- 
Stefan Richter
-=====-==--= --== -=-=-
http://arcgraph.de/sr/