[Xenomai] [PATCH 1/2] rtcan_module: Fix tx_timeout buffer size inconsistency

[Xenomai] [PATCH 1/2] rtcan_module: Fix tx_timeout buffer size inconsistency

[Paul Janzen]

When formatting /proc/rtcan/sockets, tx_timeout is defined as
char[16], but a length of 20 is passed to rtcan_get_timeout_name. If
tx_timeout for any socket is infinite, this causes snprintf to zero
outside the buffer, leading to stack overflow and a kernel panic.
Make the tx_timeout buffer 20 bytes long instead.

Signed-off-by: Paul Janzen <pcj@xenomai.sez.to>
---
 ksrc/drivers/can/rtcan_module.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/ksrc/drivers/can/rtcan_module.c b/ksrc/drivers/can/rtcan_module.c
index 3ebc333..f270f65 100644
--- a/ksrc/drivers/can/rtcan_module.c
+++ b/ksrc/drivers/can/rtcan_module.c
@@ -165,7 +165,7 @@ static int rtcan_read_proc_sockets(char *buf, char **start, off_t offset,
     struct rtdm_dev_context *context;
     struct rtcan_device *dev;
     char name[IFNAMSIZ] = "not-bound";
-    char rx_timeout[20], tx_timeout[16];
+    char rx_timeout[20], tx_timeout[20];
     rtdm_lockctx_t lock_ctx;
     int ifindex;
     RTCAN_PROC_PRINT_VARS(120);
-- 
1.7.6.5

[Xenomai] [PATCH 2/2] rtcan_module: Improve buffer size handling for /proc/rtcan entries

[Paul Janzen]

Signed-off-by: Paul Janzen <pcj@xenomai.sez.to>
---
 ksrc/drivers/can/rtcan_module.c |   26 +++++++++++++++++---------
 1 files changed, 17 insertions(+), 9 deletions(-)

diff --git a/ksrc/drivers/can/rtcan_module.c b/ksrc/drivers/can/rtcan_module.c
index f270f65..6301f63 100644
--- a/ksrc/drivers/can/rtcan_module.c
+++ b/ksrc/drivers/can/rtcan_module.c
@@ -116,7 +116,7 @@ static void rtcan_get_timeout_name(nanosecs_rel_t timeout,
     if (timeout == RTDM_TIMEOUT_INFINITE)
 	strncpy(name, "infinite", max_len);
     else
-	sprintf(name, "%lld", (long long)timeout);
+	snprintf(name, max_len, "%lld", (long long)timeout);
 }
 
 static int rtcan_read_proc_devices(char *buf, char **start, off_t offset,
@@ -141,8 +141,10 @@ static int rtcan_read_proc_devices(char *buf, char **start, off_t offset,
 
     for (i = 1; i <= RTCAN_MAX_DEVICES; i++) {
 	if ((dev = rtcan_dev_get_by_index(i)) != NULL) {
-	    rtcan_dev_get_state_name(dev->state, state_name, 20);
-	    rtcan_dev_get_baudrate_name(dev->baudrate, baudrate_name, 20);
+	    rtcan_dev_get_state_name(dev->state,
+				     state_name, sizeof(state_name));
+	    rtcan_dev_get_baudrate_name(dev->baudrate,
+					baudrate_name, sizeof(baudrate_name));
 	    ret = RTCAN_PROC_PRINT("%-15s %9s %-8s %10d %10d %10d\n",
 				   dev->name, baudrate_name, state_name,
 				   dev->tx_count, dev->rx_count, dev->err_count);
@@ -195,8 +197,10 @@ static int rtcan_read_proc_sockets(char *buf, char **start, off_t offset,
 	    } else
 		sprintf(name, "%d", ifindex);
 	}
-	rtcan_get_timeout_name(sock->tx_timeout, tx_timeout, 20);
-	rtcan_get_timeout_name(sock->rx_timeout, rx_timeout, 20);
+	rtcan_get_timeout_name(sock->tx_timeout,
+			       tx_timeout, sizeof(tx_timeout));
+	rtcan_get_timeout_name(sock->rx_timeout,
+			       rx_timeout, sizeof(rx_timeout));
 	if (!RTCAN_PROC_PRINT("%2d %-15s %6d 0x%05x %13s %13s %10d %5d\n",
 			      context->fd, name, sock->flistlen,
 			      sock->err_mask, rx_timeout, tx_timeout,
@@ -224,10 +228,14 @@ static int rtcan_read_proc_info(char *buf, char **start, off_t offset,
     if (down_interruptible(&rtcan_devices_nrt_lock))
 	return -ERESTARTSYS;
 
-    rtcan_dev_get_state_name(dev->state, state_name, 20);
-    rtcan_dev_get_ctrlmode_name(dev->ctrl_mode, ctrlmode_name, 80);
-    rtcan_dev_get_baudrate_name(dev->baudrate, baudrate_name, 20);
-    rtcan_dev_get_bittime_name(&dev->bit_time, bittime_name, 80);
+    rtcan_dev_get_state_name(dev->state,
+			     state_name, sizeof(state_name));
+    rtcan_dev_get_ctrlmode_name(dev->ctrl_mode,
+				ctrlmode_name, sizeof(ctrlmode_name));
+    rtcan_dev_get_baudrate_name(dev->baudrate,
+				baudrate_name, sizeof(baudrate_name));
+    rtcan_dev_get_bittime_name(&dev->bit_time,
+			       bittime_name, sizeof(bittime_name));
 
     if (!RTCAN_PROC_PRINT("%s %s\n", "Device    ", dev->name) ||
 	!RTCAN_PROC_PRINT("%s %s\n", "Controller", dev->ctrl_name) ||
-- 
1.7.6.5

Re: [Xenomai] [PATCH 1/2] rtcan_module: Fix tx_timeout buffer size inconsistency

[Gilles Chanteperdrix]

On 03/17/2013 07:56 PM, Paul Janzen wrote:

> When formatting /proc/rtcan/sockets, tx_timeout is defined as
> char[16], but a length of 20 is passed to rtcan_get_timeout_name. If
> tx_timeout for any socket is infinite, this causes snprintf to zero
> outside the buffer, leading to stack overflow and a kernel panic.
> Make the tx_timeout buffer 20 bytes long instead.
> 
> Signed-off-by: Paul Janzen <pcj@xenomai.sez.to>


Wolfgang, ping?

-- 
                                                                Gilles.

Re: [Xenomai] [PATCH 1/2] rtcan_module: Fix tx_timeout buffer size inconsistency

[Wolfgang Grandegger]

On 04/09/2013 09:32 PM, Gilles Chanteperdrix wrote:
> On 03/17/2013 07:56 PM, Paul Janzen wrote:
> 
>> When formatting /proc/rtcan/sockets, tx_timeout is defined as
>> char[16], but a length of 20 is passed to rtcan_get_timeout_name. If
>> tx_timeout for any socket is infinite, this causes snprintf to zero
>> outside the buffer, leading to stack overflow and a kernel panic.
>> Make the tx_timeout buffer 20 bytes long instead.
>>
>> Signed-off-by: Paul Janzen <pcj@xenomai.sez.to>

It's a bug, therefore:

Acked-by: Wolfgang Grandegger <wg@grandegger.com>

> Wolfgang, ping?

Pong, sorry for delay.

Wolfgang.

Re: [Xenomai] [PATCH 1/2] rtcan_module: Fix tx_timeout buffer size inconsistency

[Gilles Chanteperdrix]

On 04/10/2013 12:05 PM, Wolfgang Grandegger wrote:

> On 04/09/2013 09:32 PM, Gilles Chanteperdrix wrote:
>> On 03/17/2013 07:56 PM, Paul Janzen wrote:
>>
>>> When formatting /proc/rtcan/sockets, tx_timeout is defined as
>>> char[16], but a length of 20 is passed to rtcan_get_timeout_name. If
>>> tx_timeout for any socket is infinite, this causes snprintf to zero
>>> outside the buffer, leading to stack overflow and a kernel panic.
>>> Make the tx_timeout buffer 20 bytes long instead.
>>>
>>> Signed-off-by: Paul Janzen <pcj@xenomai.sez.to>
> 
> It's a bug, therefore:
> 
> Acked-by: Wolfgang Grandegger <wg@grandegger.com>
> 
>> Wolfgang, ping?
> 
> Pong, sorry for delay.


Hi,

No problem for the delay, merged, thanks.

We have two other unconfirmed CAN patches:
http://www.xenomai.org/pipermail/xenomai/2013-March/028111.html

And not really a patch, but modifying the code is trivial if you confirm
that we can do it:
http://www.xenomai.org/pipermail/xenomai/2013-February/027824.html

Regards.

-- 
                                                                Gilles.

Re: [Xenomai] [PATCH 1/2] rtcan_module: Fix tx_timeout buffer size inconsistency

[Wolfgang Grandegger]

On 04/14/2013 05:04 PM, Gilles Chanteperdrix wrote:
> On 04/10/2013 12:05 PM, Wolfgang Grandegger wrote:
> 
>> On 04/09/2013 09:32 PM, Gilles Chanteperdrix wrote:
>>> On 03/17/2013 07:56 PM, Paul Janzen wrote:
>>>
>>>> When formatting /proc/rtcan/sockets, tx_timeout is defined as
>>>> char[16], but a length of 20 is passed to rtcan_get_timeout_name. If
>>>> tx_timeout for any socket is infinite, this causes snprintf to zero
>>>> outside the buffer, leading to stack overflow and a kernel panic.
>>>> Make the tx_timeout buffer 20 bytes long instead.
>>>>
>>>> Signed-off-by: Paul Janzen <pcj@xenomai.sez.to>
>>
>> It's a bug, therefore:
>>
>> Acked-by: Wolfgang Grandegger <wg@grandegger.com>
>>
>>> Wolfgang, ping?
>>
>> Pong, sorry for delay.
> 
> 
> Hi,
> 
> No problem for the delay, merged, thanks.
> 
> We have two other unconfirmed CAN patches:
> http://www.xenomai.org/pipermail/xenomai/2013-March/028111.html

We can add even more PCI IDs here. I'm going to prepare a patch. Just
support for 4 channel cards needs more effort.

> And not really a patch, but modifying the code is trivial if you confirm
> that we can do it:

> http://www.xenomai.org/pipermail/xenomai/2013-February/027824.html

Well, I'm not happy with that extension to rtcansend. I will answer to
the original thread.

Wolfgang.

Re: [Xenomai] [PATCH 1/2] rtcan_module: Fix tx_timeout buffer size inconsistency

[Paul Janzen]

Wolfgang Grandegger <wg@grandegger.com> writes:

>> We have two other unconfirmed CAN patches:
>> http://www.xenomai.org/pipermail/xenomai/2013-March/028111.html
>
> We can add even more PCI IDs here. I'm going to prepare a patch. Just
> support for 4 channel cards needs more effort.

Along these same lines, would there be any interest in a patch
removing the deprecated ESD PCI-CAN driver?

All of the devices claimed by that driver are supported by PLX_PCI.

-- Paul

Re: [Xenomai] [PATCH 1/2] rtcan_module: Fix tx_timeout buffer size inconsistency

[Wolfgang Grandegger]

Hi Paul,

On 04/15/2013 09:18 AM, Paul Janzen wrote:
> Wolfgang Grandegger <wg@grandegger.com> writes:
> 
>>> We have two other unconfirmed CAN patches:
>>> http://www.xenomai.org/pipermail/xenomai/2013-March/028111.html
>>
>> We can add even more PCI IDs here. I'm going to prepare a patch. Just
>> support for 4 channel cards needs more effort.
> 
> Along these same lines, would there be any interest in a patch
> removing the deprecated ESD PCI-CAN driver?

It does not make a lot of sense to maintain two variants of a driver.

> All of the devices claimed by that driver are supported by PLX_PCI.

Yes, I will check if there are some relevant differences.

Wolfgang.