Possible race in nsc-ircc.ko

[Anton Volkov]

Hello.

While searching for races in the Linux kernel I've come across 
"drivers/net/irda/nsc-ircc.ko" module. Here is a question that I came up 
with while analyzing results. Lines are given using the info from Linux 
v4.12.


Consider the following case:

Thread 1:   Thread 2:
nsc_ircc_init
->nsc_ircc_open
self = netdev_priv(dev)
register_netdev(dev)
nsc_ircc_net_ioctl
->nsc_ircc_change_speed
self->dongle_id = ...   io.dongle_id>
(nsc-ircc.c: line 485)  (nsc-ircc.c: line 1318)
platform_device_register_simple

Before the initialization of self->dongle_id in msc_ircc_open() its 
value is 0. Thus if read access to its value in nsc_ircc_change_speed 
occurs before the initialization there will be an attempt to change 
speed of dongle with undesired id (if the dongle with id 0 exists). Is 
this case feasible from your point of view?


Thank you for your time.

-- Anton Volkov
Linux Verification Center, ISPRAS
web: http://linuxtesting.org
e-mail: avol...@ispras.ru

Possible race in ks8851_mll.ko

[Anton Volkov]

Hello.

While searching for races in the Linux kernel I've come across
"drivers/net/ethernet/micrel/ks8851_mll.ko" module. Here is a question 
that I came up with while analyzing results. Lines are given using the 
info from Linux v4.12.


Consider the following case:

Thread 1:Thread 2:
ks8851_probe
  ks = netdev_priv(netdev)
  register_netdev
 ks_start_xmit
->ks_soft_reset  ->ks_tx_fifo_space
  ->ks_wrreg16 ->ks_rdreg16
  ks->cmd_reg_cache = ...  ks->cmd_reg_cache = ...
  (ks8851_mll.c: line 536) (ks8851_mll.c: line 505)
  iowrite16(ks->cmd_reg_cache) iowrite16(ks->cmd_reg_cache)

In this case early registration of netdev leads to callback interference 
in the initialization process. Both ks_wrreg16() and ks_rdreg16() use 
the same ks. If one of them changes the ks->cmd_reg_cache it is possible 
that both will use the same value though it should be different. Is this 
race feasible from your point of view?


Thank you for your time.

-- Anton Volkov
Linux Verification Center, ISPRAS
web: http://linuxtesting.org
e-mail: avol...@ispras.ru

Possible race in via-ircc.ko

[Anton Volkov]

Hello.

While searching for races in the Linux kernel I've come across 
"drivers/net/irda/via-ircc.ko" module. Here are questions that I came up 
with while analyzing results. Lines are given using the info from Linux 
v4.12.


Consider the following case:

Thread 1:Thread 2:
via_ircc_net_open
  request_irq
  
 via_ircc_interrupt
-> via_ircc_dma_receive  -> RxTimerHandler
   (via-ircc.c: line 1488)  (via-ircc.c: line 1315)
 self->... = ...  ... = self->...

In the via_ircc_dma_receive a lot of fields of 'self' structure are 
initialized and via_ircc_interrupt with RxTimerHandler use those fields. 
If no initialization happened interrupt handler and other functions that 
it calls may work with incorrect data. I'm not sure how bad this case 
can be and thus here are my questions. Is this situation feasible from 
your point of view? If it is feasible, is it a benign race or something 
serious?


Thank you for your time.

-- Anton Volkov
Linux Verification Center, ISPRAS
web: http://linuxtesting.org
e-mail: avol...@ispras.ru

Possible race in c4.ko

[Anton Volkov]

Hello.

While searching for races in the Linux kernel I've come across 
"drivers/isdn/hardware/avm/c4.ko" module. Here is a question that I came 
up with while analyzing results. Lines are given using the info from 
Linux v4.12.


Consider the following case:

Thread 1:  Thread 2:
c4_probe
->c4_add_card
request_irq()
   c4_interrupt
   ->c4_handle_interrupt
 ->c4_handle_rx
card->cardnr = ... cidx = f(card->cardnr)
(c4.c: line 1227)  (c4.c: line 526)
   if (cidx >= card->nlogcontr) cidx = 0;
   ctrl = >ctrlinfo[cidx].capi_ctrl

card->cardnr is 0 until it is initialized in c4_add_card(). If at the 
moment of read access in c4_handle_rx() it is still 0, cidx may then be 
assigned an undesirable value and wrong controller may handle messages. 
Is this case feasible from your point of view?


Thank you for your time.

-- Anton Volkov
Linux Verification Center, ISPRAS
web: http://linuxtesting.org
e-mail: avol...@ispras.ru

Question about via-ircc.ko

[Anton Volkov]

Hello.

While searching for races in the Linux kernel I've come across 
"drivers/net/irda/via-ircc.ko" module. Here are questions that I came up 
with while analyzing results. Lines are given using the info from Linux 
v4.12.


Consider the following case:

Thread 1:Thread 2:
via_ircc_net_open
  request_irq
  
 via_ircc_interrupt
-> via_ircc_dma_receive  -> RxTimerHandler
   (via-ircc.c: line 1488)  (via-ircc.c: line 1315)
 self->... = ...  ... = self->...

In the via_ircc_dma_receive a lot of fields of 'self' structure are 
initialized and via_ircc_interrupt with RxTimerHandler use those fields. 
If no initialization happened interrupt handler and other functions that 
it calls may work with incorrect data. I'm not sure how bad this case 
can be and thus here are my questions. Is this situation feasible from 
your point of view? If it is feasible, is it a benign race or something 
serious?


Thank you for your time.

-- Anton Volkov
Linux Verification Center, ISPRAS
web: http://linuxtesting.org
e-mail: avol...@ispras.ru

[PATCH v2] hysdn: fix to a race condition in put_log_buffer

[Anton Volkov]

The synchronization type that was used earlier to guard the loop that
deletes unused log buffers may lead to a situation that prevents any
thread from going through the loop.

The patch deletes previously used synchronization mechanism and moves
the loop under the spin_lock so the similar cases won't be feasible in
the future.

Found by by Linux Driver Verification project (linuxtesting.org).

Signed-off-by: Anton Volkov <avol...@ispras.ru>
---
v2: Fixed coding style issues

 drivers/isdn/hysdn/hysdn_proclog.c | 28 +---
 1 file changed, 13 insertions(+), 15 deletions(-)

diff --git a/drivers/isdn/hysdn/hysdn_proclog.c 
b/drivers/isdn/hysdn/hysdn_proclog.c
index 7b5fd8f..aaca0b3 100644
--- a/drivers/isdn/hysdn/hysdn_proclog.c
+++ b/drivers/isdn/hysdn/hysdn_proclog.c
@@ -44,7 +44,6 @@ struct procdata {
char log_name[15];  /* log filename */
struct log_data *log_head, *log_tail;   /* head and tail for queue */
int if_used;/* open count for interface */
-   int volatile del_lock;  /* lock for delete operations */
unsigned char logtmp[LOG_MAX_LINELEN];
wait_queue_head_t rd_queue;
 };
@@ -102,7 +101,6 @@ put_log_buffer(hysdn_card *card, char *cp)
 {
struct log_data *ib;
struct procdata *pd = card->proclog;
-   int i;
unsigned long flags;
 
if (!pd)
@@ -126,21 +124,21 @@ put_log_buffer(hysdn_card *card, char *cp)
else
pd->log_tail->next = ib;/* follows existing messages */
pd->log_tail = ib;  /* new tail */
-   i = pd->del_lock++; /* get lock state */
-   spin_unlock_irqrestore(>hysdn_lock, flags);
 
/* delete old entrys */
-   if (!i)
-   while (pd->log_head->next) {
-   if ((pd->log_head->usage_cnt <= 0) &&
-   (pd->log_head->next->usage_cnt <= 0)) {
-   ib = pd->log_head;
-   pd->log_head = pd->log_head->next;
-   kfree(ib);
-   } else
-   break;
-   }   /* pd->log_head->next */
-   pd->del_lock--; /* release lock level */
+   while (pd->log_head->next) {
+   if ((pd->log_head->usage_cnt <= 0) &&
+   (pd->log_head->next->usage_cnt <= 0)) {
+   ib = pd->log_head;
+   pd->log_head = pd->log_head->next;
+   kfree(ib);
+   } else {
+   break;
+   }
+   }   /* pd->log_head->next */
+
+   spin_unlock_irqrestore(>hysdn_lock, flags);
+
wake_up_interruptible(&(pd->rd_queue)); /* announce new entry */
 }  /* put_log_buffer */
 
-- 
2.7.4

[PATCH] hysdn: fix to a race condition in put_log_buffer

[Anton Volkov]

The synchronization type that was used earlier to guard the loop that
deletes unused log buffers may lead to a situation that prevents any
thread from going through the loop.

The patch deletes previously used synchronization mechanism and moves
the loop under the spin_lock so the similar cases won't be feasible in
the future.

Found by by Linux Driver Verification project (linuxtesting.org).

Signed-off-by: Anton Volkov <avol...@ispras.ru>
---
 drivers/isdn/hysdn/hysdn_proclog.c | 27 ---
 1 file changed, 12 insertions(+), 15 deletions(-)

diff --git a/drivers/isdn/hysdn/hysdn_proclog.c 
b/drivers/isdn/hysdn/hysdn_proclog.c
index 7b5fd8f..b152c6c 100644
--- a/drivers/isdn/hysdn/hysdn_proclog.c
+++ b/drivers/isdn/hysdn/hysdn_proclog.c
@@ -44,7 +44,6 @@ struct procdata {
char log_name[15];  /* log filename */
struct log_data *log_head, *log_tail;   /* head and tail for queue */
int if_used;/* open count for interface */
-   int volatile del_lock;  /* lock for delete operations */
unsigned char logtmp[LOG_MAX_LINELEN];
wait_queue_head_t rd_queue;
 };
@@ -102,7 +101,6 @@ put_log_buffer(hysdn_card *card, char *cp)
 {
struct log_data *ib;
struct procdata *pd = card->proclog;
-   int i;
unsigned long flags;
 
if (!pd)
@@ -126,21 +124,20 @@ put_log_buffer(hysdn_card *card, char *cp)
else
pd->log_tail->next = ib;/* follows existing messages */
pd->log_tail = ib;  /* new tail */
-   i = pd->del_lock++; /* get lock state */
-   spin_unlock_irqrestore(>hysdn_lock, flags);
 
/* delete old entrys */
-   if (!i)
-   while (pd->log_head->next) {
-   if ((pd->log_head->usage_cnt <= 0) &&
-   (pd->log_head->next->usage_cnt <= 0)) {
-   ib = pd->log_head;
-   pd->log_head = pd->log_head->next;
-   kfree(ib);
-   } else
-   break;
-   }   /* pd->log_head->next */
-   pd->del_lock--; /* release lock level */
+   while (pd->log_head->next) {
+   if ((pd->log_head->usage_cnt <= 0) &&
+   (pd->log_head->next->usage_cnt <= 0)) {
+   ib = pd->log_head;
+   pd->log_head = pd->log_head->next;
+   kfree(ib);
+   } else
+   break;
+   }   /* pd->log_head->next */
+
+   spin_unlock_irqrestore(>hysdn_lock, flags);
+
wake_up_interruptible(&(pd->rd_queue)); /* announce new entry */
 }  /* put_log_buffer */
 
-- 
2.7.4

[PATCH] hysdn: fix to a race condition in put_log_buffer

[Anton Volkov]

The synchronization type that was used earlier to guard the loop that
deletes unused log buffers may have lead to a situation that prevents 
any thread from going through the loop.


The patch deletes previously used synchronization mechanism and moves
the loop under the spin_lock so the similar cases won't be feasible in
the future.

Found by by Linux Driver Verification project (linuxtesting.org).

Signed-off-by: Anton Volkov <avol...@ispras.ru>
---
 drivers/isdn/hysdn/hysdn_proclog.c | 27 ---
 1 file changed, 12 insertions(+), 15 deletions(-)

diff --git a/drivers/isdn/hysdn/hysdn_proclog.c 
b/drivers/isdn/hysdn/hysdn_proclog.c

index 7b5fd8fb1761..b152c6ca444b 100644
--- a/drivers/isdn/hysdn/hysdn_proclog.c
+++ b/drivers/isdn/hysdn/hysdn_proclog.c
@@ -44,7 +44,6 @@ struct procdata {
char log_name[15];  /* log filename */
struct log_data *log_head, *log_tail;   /* head and tail for queue */
int if_used;/* open count for interface */
-   int volatile del_lock;  /* lock for delete operations */
unsigned char logtmp[LOG_MAX_LINELEN];
wait_queue_head_t rd_queue;
 };
@@ -102,7 +101,6 @@ put_log_buffer(hysdn_card *card, char *cp)
 {
struct log_data *ib;
struct procdata *pd = card->proclog;
-   int i;
unsigned long flags;

if (!pd)
@@ -126,21 +124,20 @@ put_log_buffer(hysdn_card *card, char *cp)
else
pd->log_tail->next = ib;  /* follows existing messages */
pd->log_tail = ib;   /* new tail */
-   i = pd->del_lock++;  /* get lock state */
-   spin_unlock_irqrestore(>hysdn_lock, flags);

/* delete old entrys */
-   if (!i)
-   while (pd->log_head->next) {
-   if ((pd->log_head->usage_cnt <= 0) &&
-   (pd->log_head->next->usage_cnt <= 0)) {
-   ib = pd->log_head;
-   pd->log_head = pd->log_head->next;
-   kfree(ib);
-   } else
-   break;
-   }   /* pd->log_head->next */
-   pd->del_lock--;  /* release lock level */
+   while (pd->log_head->next) {
+   if ((pd->log_head->usage_cnt <= 0) &&
+   (pd->log_head->next->usage_cnt <= 0)) {
+   ib = pd->log_head;
+   pd->log_head = pd->log_head->next;
+   kfree(ib);
+   } else
+   break;
+   }   /* pd->log_head->next */
+
+   spin_unlock_irqrestore(>hysdn_lock, flags);
+
wake_up_interruptible(&(pd->rd_queue));  /* announce new entry 
*/
 }  /* put_log_buffer */

--
2.11.0

Possible race in hysdn.ko

[Anton Volkov]

Hello.

While searching for races in the Linux kernel I've come across
"drivers/isdn/hysdn/hysdn.ko" module. Here is a question that I came up
with while analysing results. Lines are given using the info from Linux
v4.12.

In hysdn_proclog.c file in put_log_buffer function a non-standard type
of synchronization is employed. It uses pd->del_lock as some kind of
semaphore (hysdn_proclog.c: lines 129 and 143). Consider the following
case:

Thread 1:Thread 2:
hysdn_log_write
-> hysdn_add_log
-> put_log_buffer
 spin_lock()  hysdn_conf_open
 i = pd->del_lock++   -> hysdn_add_log
 spin_unlock()   -> put_log_buffer
 if (!i) spin_lock()
 pd->del_lock--   i = pd->del_lock++
  spin_unlock()
  if (!i) 
  pd->del_lock--

 - the loop that deletes unused buffer entries
(hysdn_proclog.c: lines 134-142).
pd->del_lock-- is not an atomic operation and is executed without any
locks. Thus it may interfere in the increment process of pd->del_lock in
another thread. There may be cases that lead to the inability of any
thread going through the .

I see several possible solutions to this problem:
1) move the  under the spin_lock and delete
pd->del_lock synchronization;
2) wrap pd->del_lock-- with spin_lock protection.

What do you think should be done about it?

Thank you for your time.