| Message ID | tencent_2C67C6D2537B236F497823BCC457976F9705@qq.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,V2] ptp: fix corrupted list in ptp_open | expand |
On Tue, Oct 31, 2023 at 06:25:42PM +0800, Edward Adam Davis wrote: > There is no lock protection when writing ptp->tsevqs in ptp_open(), > ptp_release(), which can cause data corruption, NAK. You haven't identified any actual data corruption issue. If there is an issue, please state what it is. Thanks, Richard
Hi Edward, On Tue, Oct 31, 2023 at 06:25:42PM +0800, Edward Adam Davis wrote: > There is no lock protection when writing ptp->tsevqs in ptp_open(), > ptp_release(), which can cause data corruption, use mutex lock to avoid this > issue. > > Moreover, ptp_release() should not be used to release the queue in ptp_read(), > and it should be deleted together. > > Reported-and-tested-by: syzbot+df3f3ef31f60781fa911@syzkaller.appspotmail.com > Fixes: 8f5de6fb2453 ("ptp: support multiple timestamp event readers") > Signed-off-by: Edward Adam Davis <eadavis@qq.com> > --- > drivers/ptp/ptp_chardev.c | 11 +++++++++-- > drivers/ptp/ptp_clock.c | 3 +++ > drivers/ptp/ptp_private.h | 1 + > 3 files changed, 13 insertions(+), 2 deletions(-) > > diff --git a/drivers/ptp/ptp_chardev.c b/drivers/ptp/ptp_chardev.c > index 282cd7d24077..e31551d2697d 100644 > --- a/drivers/ptp/ptp_chardev.c > +++ b/drivers/ptp/ptp_chardev.c > @@ -109,6 +109,9 @@ int ptp_open(struct posix_clock_context *pccontext, fmode_t fmode) > struct timestamp_event_queue *queue; > char debugfsname[32]; > > + if (mutex_lock_interruptible(&ptp->tsevq_mux)) > + return -ERESTARTSYS; > + > queue = kzalloc(sizeof(*queue), GFP_KERNEL); > if (!queue) > return -EINVAL; > @@ -132,15 +135,20 @@ int ptp_open(struct posix_clock_context *pccontext, fmode_t fmode) > debugfs_create_u32_array("mask", 0444, queue->debugfs_instance, > &queue->dfs_bitmap); > > + mutex_unlock(&ptp->tsevq_mux); The lock doesn't need to be held so long here. Doing so causes a bit of an issue, actually, because the memory allocation for the queue can fail which will cause the function to return early without releasing the mutex. The lock only needs to be held for the list_add_tail() call. > return 0; > } > > int ptp_release(struct posix_clock_context *pccontext) > { > struct timestamp_event_queue *queue = pccontext->private_clkdata; > + struct ptp_clock *ptp = > + container_of(pccontext->clk, struct ptp_clock, clock); > unsigned long flags; > > if (queue) { > + if (mutex_lock_interruptible(&ptp->tsevq_mux)) > + return -ERESTARTSYS; > debugfs_remove(queue->debugfs_instance); > pccontext->private_clkdata = NULL; > spin_lock_irqsave(&queue->lock, flags); > @@ -148,6 +156,7 @@ int ptp_release(struct posix_clock_context *pccontext) > spin_unlock_irqrestore(&queue->lock, flags); > bitmap_free(queue->mask); > kfree(queue); > + mutex_unlock(&ptp->tsevq_mux); Similar to the above note, you don't want to hold the lock any longer than you must. While this patch looks to cover adding and removing items from the list, the code that iterates over the list isn't covered which can be problematic. If the list is modified while it is being iterated, the iterating code could chase an invalid pointer. Regards, Jeremy
Hi Jeremy, On Thu, 2 Nov 2023 14:16:54 -0400 Jeremy Cline wrote: >> There is no lock protection when writing ptp->tsevqs in ptp_open(), >> ptp_release(), which can cause data corruption, use mutex lock to avoid this >> issue. >> >> Moreover, ptp_release() should not be used to release the queue in ptp_read(), >> and it should be deleted together. >> >> Reported-and-tested-by: syzbot+df3f3ef31f60781fa911@syzkaller.appspotmail.com >> Fixes: 8f5de6fb2453 ("ptp: support multiple timestamp event readers") >> Signed-off-by: Edward Adam Davis <eadavis@qq.com> >> --- >> drivers/ptp/ptp_chardev.c | 11 +++++++++-- >> drivers/ptp/ptp_clock.c | 3 +++ >> drivers/ptp/ptp_private.h | 1 + >> 3 files changed, 13 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/ptp/ptp_chardev.c b/drivers/ptp/ptp_chardev.c >> index 282cd7d24077..e31551d2697d 100644 >> --- a/drivers/ptp/ptp_chardev.c >> +++ b/drivers/ptp/ptp_chardev.c >> @@ -109,6 +109,9 @@ int ptp_open(struct posix_clock_context *pccontext, fmode_t fmode) >> struct timestamp_event_queue *queue; >> char debugfsname[32]; >> >> + if (mutex_lock_interruptible(&ptp->tsevq_mux)) >> + return -ERESTARTSYS; >> + >> queue = kzalloc(sizeof(*queue), GFP_KERNEL); >> if (!queue) >> return -EINVAL; >> @@ -132,15 +135,20 @@ int ptp_open(struct posix_clock_context *pccontext, fmode_t fmode) >> debugfs_create_u32_array("mask", 0444, queue->debugfs_instance, >> &queue->dfs_bitmap); >> >> + mutex_unlock(&ptp->tsevq_mux); > >The lock doesn't need to be held so long here. Doing so causes a bit of >an issue, actually, because the memory allocation for the queue can fail >which will cause the function to return early without releasing the >mutex. > >The lock only needs to be held for the list_add_tail() call. > >> return 0; >> } >> >> int ptp_release(struct posix_clock_context *pccontext) >> { >> struct timestamp_event_queue *queue = pccontext->private_clkdata; >> + struct ptp_clock *ptp = >> + container_of(pccontext->clk, struct ptp_clock, clock); >> unsigned long flags; >> >> if (queue) { >> + if (mutex_lock_interruptible(&ptp->tsevq_mux)) >> + return -ERESTARTSYS; >> debugfs_remove(queue->debugfs_instance); >> pccontext->private_clkdata = NULL; >> spin_lock_irqsave(&queue->lock, flags); >> @@ -148,6 +156,7 @@ int ptp_release(struct posix_clock_context *pccontext) >> spin_unlock_irqrestore(&queue->lock, flags); >> bitmap_free(queue->mask); >> kfree(queue); >> + mutex_unlock(&ptp->tsevq_mux); > >Similar to the above note, you don't want to hold the lock any longer >than you must. > >While this patch looks to cover adding and removing items from the list, >the code that iterates over the list isn't covered which can be >problematic. If the list is modified while it is being iterated, the >iterating code could chase an invalid pointer. Thanks for your opinions, I will double check it. Thanks, edward
On Thu, Nov 02, 2023 at 02:16:54PM -0400, Jeremy Cline wrote: > While this patch looks to cover adding and removing items from the list, > the code that iterates over the list isn't covered which can be > problematic. If the list is modified while it is being iterated, the > iterating code could chase an invalid pointer. Indeed. See ptp_clock.c: 416 case PTP_CLOCK_EXTTS: 417 /* Enqueue timestamp on selected queues */ 418 list_for_each_entry(tsevq, &ptp->tsevqs, qlist) { 419 if (test_bit((unsigned int)event->index, tsevq->mask)) 420 enqueue_external_timestamp(tsevq, event); 421 } 422 wake_up_interruptible(&ptp->tsev_wq); 423 break; Thanks, Richard
On Fri, Nov 03, 2023 at 07:13:31PM -0700, Richard Cochran wrote: > See ptp_clock.c: > > 416 case PTP_CLOCK_EXTTS: > 417 /* Enqueue timestamp on selected queues */ > 418 list_for_each_entry(tsevq, &ptp->tsevqs, qlist) { > 419 if (test_bit((unsigned int)event->index, tsevq->mask)) > 420 enqueue_external_timestamp(tsevq, event); > 421 } > 422 wake_up_interruptible(&ptp->tsev_wq); > 423 break; And that code can be called from interrupt context. Thus the mutex won't work. It needs to be a spin lock instead. Thanks, Richard
diff --git a/drivers/ptp/ptp_chardev.c b/drivers/ptp/ptp_chardev.c index 282cd7d24077..e31551d2697d 100644 --- a/drivers/ptp/ptp_chardev.c +++ b/drivers/ptp/ptp_chardev.c @@ -109,6 +109,9 @@ int ptp_open(struct posix_clock_context *pccontext, fmode_t fmode) struct timestamp_event_queue *queue; char debugfsname[32]; + if (mutex_lock_interruptible(&ptp->tsevq_mux)) + return -ERESTARTSYS; + queue = kzalloc(sizeof(*queue), GFP_KERNEL); if (!queue) return -EINVAL; @@ -132,15 +135,20 @@ int ptp_open(struct posix_clock_context *pccontext, fmode_t fmode) debugfs_create_u32_array("mask", 0444, queue->debugfs_instance, &queue->dfs_bitmap); + mutex_unlock(&ptp->tsevq_mux); return 0; } int ptp_release(struct posix_clock_context *pccontext) { struct timestamp_event_queue *queue = pccontext->private_clkdata; + struct ptp_clock *ptp = + container_of(pccontext->clk, struct ptp_clock, clock); unsigned long flags; if (queue) { + if (mutex_lock_interruptible(&ptp->tsevq_mux)) + return -ERESTARTSYS; debugfs_remove(queue->debugfs_instance); pccontext->private_clkdata = NULL; spin_lock_irqsave(&queue->lock, flags); @@ -148,6 +156,7 @@ int ptp_release(struct posix_clock_context *pccontext) spin_unlock_irqrestore(&queue->lock, flags); bitmap_free(queue->mask); kfree(queue); + mutex_unlock(&ptp->tsevq_mux); } return 0; } @@ -585,7 +594,5 @@ ssize_t ptp_read(struct posix_clock_context *pccontext, uint rdflags, free_event: kfree(event); exit: - if (result < 0) - ptp_release(pccontext); return result; } diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c index 3d1b0a97301c..7930db6ec18d 100644 --- a/drivers/ptp/ptp_clock.c +++ b/drivers/ptp/ptp_clock.c @@ -176,6 +176,7 @@ static void ptp_clock_release(struct device *dev) ptp_cleanup_pin_groups(ptp); kfree(ptp->vclock_index); + mutex_destroy(&ptp->tsevq_mux); mutex_destroy(&ptp->pincfg_mux); mutex_destroy(&ptp->n_vclocks_mux); /* Delete first entry */ @@ -247,6 +248,7 @@ struct ptp_clock *ptp_clock_register(struct ptp_clock_info *info, if (!queue) goto no_memory_queue; list_add_tail(&queue->qlist, &ptp->tsevqs); + mutex_init(&ptp->tsevq_mux); queue->mask = bitmap_alloc(PTP_MAX_CHANNELS, GFP_KERNEL); if (!queue->mask) goto no_memory_bitmap; @@ -356,6 +358,7 @@ struct ptp_clock *ptp_clock_register(struct ptp_clock_info *info, if (ptp->kworker) kthread_destroy_worker(ptp->kworker); kworker_err: + mutex_destroy(&ptp->tsevq_mux); mutex_destroy(&ptp->pincfg_mux); mutex_destroy(&ptp->n_vclocks_mux); bitmap_free(queue->mask); diff --git a/drivers/ptp/ptp_private.h b/drivers/ptp/ptp_private.h index 52f87e394aa6..1525bd2059ba 100644 --- a/drivers/ptp/ptp_private.h +++ b/drivers/ptp/ptp_private.h @@ -44,6 +44,7 @@ struct ptp_clock { struct pps_device *pps_source; long dialed_frequency; /* remembers the frequency adjustment */ struct list_head tsevqs; /* timestamp fifo list */ + struct mutex tsevq_mux; /* one process at a time reading the fifo */ struct mutex pincfg_mux; /* protect concurrent info->pin_config access */ wait_queue_head_t tsev_wq; int defunct; /* tells readers to go away when clock is being removed */
There is no lock protection when writing ptp->tsevqs in ptp_open(), ptp_release(), which can cause data corruption, use mutex lock to avoid this issue. Moreover, ptp_release() should not be used to release the queue in ptp_read(), and it should be deleted together. Reported-and-tested-by: syzbot+df3f3ef31f60781fa911@syzkaller.appspotmail.com Fixes: 8f5de6fb2453 ("ptp: support multiple timestamp event readers") Signed-off-by: Edward Adam Davis <eadavis@qq.com> --- drivers/ptp/ptp_chardev.c | 11 +++++++++-- drivers/ptp/ptp_clock.c | 3 +++ drivers/ptp/ptp_private.h | 1 + 3 files changed, 13 insertions(+), 2 deletions(-)