lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <CE56425C.6A0B3%andreas.dilger@intel.com>
Date:	Wed, 11 Sep 2013 22:00:03 +0000
From:	"Dilger, Andreas" <andreas.dilger@...el.com>
To:	Kees Cook <keescook@...omium.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
CC:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Peng Tao <bergwolf@...il.com>,
	Masanari Iida <standby24x7@...il.com>,
	"Drokin, Oleg" <oleg.drokin@...el.com>,
	"Mannthey, Keith" <keith.mannthey@...el.com>,
	"Eremin, Dmitry" <dmitry.eremin@...el.com>,
	Emil Goode <emilgoode@...il.com>,
	Nikitas Angelinas <nikitas_angelinas@...atex.com>,
	"devel@...verdev.osuosl.org" <devel@...verdev.osuosl.org>
Subject: Re: [PATCH] staging: lustre: clean up format string usages

On 2013/09/10 10:37 PM, "Kees Cook" <keescook@...omium.org> wrote:
>This fixes up the usage of snprintf, strncpy, and format strings in the
>call to kthread_run to avoid ever accidentally allowing a format string
>into the thread name.
>
>Signed-off-by: Kees Cook <keescook@...omium.org>

No objection, though I don't think there was any security risk here.
All of these thread names originate in the kernel.

Cheers, Andreas

>---
> .../staging/lustre/lnet/klnds/o2iblnd/o2iblnd_cb.c |    2 +-
> .../staging/lustre/lnet/klnds/socklnd/socklnd_cb.c |    2 +-
> drivers/staging/lustre/lustre/ldlm/ldlm_lockd.c    |    4 ++--
> drivers/staging/lustre/lustre/libcfs/workitem.c    |    2 +-
> drivers/staging/lustre/lustre/ptlrpc/pinger.c      |    4 ++--
> drivers/staging/lustre/lustre/ptlrpc/ptlrpcd.c     |    8 ++++----
> drivers/staging/lustre/lustre/ptlrpc/service.c     |    6 +++---
> 7 files changed, 14 insertions(+), 14 deletions(-)
>
>diff --git a/drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd_cb.c
>b/drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd_cb.c
>index 086ca3d..26b49a2 100644
>--- a/drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd_cb.c
>+++ b/drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd_cb.c
>@@ -1802,7 +1802,7 @@ kiblnd_recv (lnet_ni_t *ni, void *private,
>lnet_msg_t *lntmsg, int delayed,
> int
> kiblnd_thread_start(int (*fn)(void *arg), void *arg, char *name)
> {
>-	struct task_struct *task = kthread_run(fn, arg, name);
>+	struct task_struct *task = kthread_run(fn, arg, "%s", name);
> 
> 	if (IS_ERR(task))
> 		return PTR_ERR(task);
>diff --git a/drivers/staging/lustre/lnet/klnds/socklnd/socklnd_cb.c
>b/drivers/staging/lustre/lnet/klnds/socklnd/socklnd_cb.c
>index 2c581b7..68a4f52 100644
>--- a/drivers/staging/lustre/lnet/klnds/socklnd/socklnd_cb.c
>+++ b/drivers/staging/lustre/lnet/klnds/socklnd/socklnd_cb.c
>@@ -1005,7 +1005,7 @@ ksocknal_send(lnet_ni_t *ni, void *private,
>lnet_msg_t *lntmsg)
> int
> ksocknal_thread_start(int (*fn)(void *arg), void *arg, char *name)
> {
>-	struct task_struct *task = kthread_run(fn, arg, name);
>+	struct task_struct *task = kthread_run(fn, arg, "%s", name);
> 
> 	if (IS_ERR(task))
> 		return PTR_ERR(task);
>diff --git a/drivers/staging/lustre/lustre/ldlm/ldlm_lockd.c
>b/drivers/staging/lustre/lustre/ldlm/ldlm_lockd.c
>index 3916bda..a100a0b 100644
>--- a/drivers/staging/lustre/lustre/ldlm/ldlm_lockd.c
>+++ b/drivers/staging/lustre/lustre/ldlm/ldlm_lockd.c
>@@ -800,9 +800,9 @@ static int ldlm_bl_thread_start(struct ldlm_bl_pool
>*blp)
> 
> 	init_completion(&bltd.bltd_comp);
> 	bltd.bltd_num = atomic_read(&blp->blp_num_threads);
>-	snprintf(bltd.bltd_name, sizeof(bltd.bltd_name) - 1,
>+	snprintf(bltd.bltd_name, sizeof(bltd.bltd_name),
> 		"ldlm_bl_%02d", bltd.bltd_num);
>-	task = kthread_run(ldlm_bl_thread_main, &bltd, bltd.bltd_name);
>+	task = kthread_run(ldlm_bl_thread_main, &bltd, "%s", bltd.bltd_name);
> 	if (IS_ERR(task)) {
> 		CERROR("cannot start LDLM thread ldlm_bl_%02d: rc %ld\n",
> 		       atomic_read(&blp->blp_num_threads), PTR_ERR(task));
>diff --git a/drivers/staging/lustre/lustre/libcfs/workitem.c
>b/drivers/staging/lustre/lustre/libcfs/workitem.c
>index 462172d..1a55c81 100644
>--- a/drivers/staging/lustre/lustre/libcfs/workitem.c
>+++ b/drivers/staging/lustre/lustre/libcfs/workitem.c
>@@ -397,7 +397,7 @@ cfs_wi_sched_create(char *name, struct cfs_cpt_table
>*cptab,
> 				 sched->ws_name, sched->ws_nthreads);
> 		}
> 
>-		task = kthread_run(cfs_wi_scheduler, sched, name);
>+		task = kthread_run(cfs_wi_scheduler, sched, "%s", name);
> 		if (!IS_ERR(task)) {
> 			nthrs--;
> 			continue;
>diff --git a/drivers/staging/lustre/lustre/ptlrpc/pinger.c
>b/drivers/staging/lustre/lustre/ptlrpc/pinger.c
>index 227a0ae..5dec771 100644
>--- a/drivers/staging/lustre/lustre/ptlrpc/pinger.c
>+++ b/drivers/staging/lustre/lustre/ptlrpc/pinger.c
>@@ -383,8 +383,8 @@ int ptlrpc_start_pinger(void)
> 
> 	/* CLONE_VM and CLONE_FILES just avoid a needless copy, because we
> 	 * just drop the VM and FILES in cfs_daemonize_ctxt() right away. */
>-	rc = PTR_ERR(kthread_run(ptlrpc_pinger_main,
>-				 &pinger_thread, pinger_thread.t_name));
>+	rc = PTR_ERR(kthread_run(ptlrpc_pinger_main, &pinger_thread,
>+				 "%s", pinger_thread.t_name));
> 	if (IS_ERR_VALUE(rc)) {
> 		CERROR("cannot start thread: %d\n", rc);
> 		return rc;
>diff --git a/drivers/staging/lustre/lustre/ptlrpc/ptlrpcd.c
>b/drivers/staging/lustre/lustre/ptlrpc/ptlrpcd.c
>index fbdeff6..89c9be9 100644
>--- a/drivers/staging/lustre/lustre/ptlrpc/ptlrpcd.c
>+++ b/drivers/staging/lustre/lustre/ptlrpc/ptlrpcd.c
>@@ -615,7 +615,7 @@ int ptlrpcd_start(int index, int max, const char
>*name, struct ptlrpcd_ctl *pc)
> 	init_completion(&pc->pc_starting);
> 	init_completion(&pc->pc_finishing);
> 	spin_lock_init(&pc->pc_lock);
>-	strncpy(pc->pc_name, name, sizeof(pc->pc_name) - 1);
>+	strlcpy(pc->pc_name, name, sizeof(pc->pc_name));
> 	pc->pc_set = ptlrpc_prep_set();
> 	if (pc->pc_set == NULL)
> 		GOTO(out, rc = -ENOMEM);
>@@ -638,7 +638,7 @@ int ptlrpcd_start(int index, int max, const char
>*name, struct ptlrpcd_ctl *pc)
> 				GOTO(out, rc);
> 		}
> 
>-		task = kthread_run(ptlrpcd, pc, pc->pc_name);
>+		task = kthread_run(ptlrpcd, pc, "%s", pc->pc_name);
> 		if (IS_ERR(task))
> 			GOTO(out, rc = PTR_ERR(task));
> 
>@@ -745,7 +745,7 @@ static int ptlrpcd_init(void)
> 	if (ptlrpcds == NULL)
> 		GOTO(out, rc = -ENOMEM);
> 
>-	snprintf(name, 15, "ptlrpcd_rcv");
>+	snprintf(name, sizeof(name), "ptlrpcd_rcv");
> 	set_bit(LIOD_RECOVERY, &ptlrpcds->pd_thread_rcv.pc_flags);
> 	rc = ptlrpcd_start(-1, nthreads, name, &ptlrpcds->pd_thread_rcv);
> 	if (rc < 0)
>@@ -764,7 +764,7 @@ static int ptlrpcd_init(void)
> 	 *      unnecessary dependency. But how to distribute async RPCs load
> 	 *      among all the ptlrpc daemons becomes another trouble. */
> 	for (i = 0; i < nthreads; i++) {
>-		snprintf(name, 15, "ptlrpcd_%d", i);
>+		snprintf(name, sizeof(name), "ptlrpcd_%d", i);
> 		rc = ptlrpcd_start(i, nthreads, name, &ptlrpcds->pd_threads[i]);
> 		if (rc < 0)
> 			GOTO(out, rc);
>diff --git a/drivers/staging/lustre/lustre/ptlrpc/service.c
>b/drivers/staging/lustre/lustre/ptlrpc/service.c
>index ac8b5fd..acf75f3 100644
>--- a/drivers/staging/lustre/lustre/ptlrpc/service.c
>+++ b/drivers/staging/lustre/lustre/ptlrpc/service.c
>@@ -2718,15 +2718,15 @@ int ptlrpc_start_thread(struct
>ptlrpc_service_part *svcpt, int wait)
> 	spin_unlock(&svcpt->scp_lock);
> 
> 	if (svcpt->scp_cpt >= 0) {
>-		snprintf(thread->t_name, PTLRPC_THR_NAME_LEN, "%s%02d_%03d",
>+		snprintf(thread->t_name, sizeof(thread->t_name), "%s%02d_%03d",
> 			 svc->srv_thread_name, svcpt->scp_cpt, thread->t_id);
> 	} else {
>-		snprintf(thread->t_name, PTLRPC_THR_NAME_LEN, "%s_%04d",
>+		snprintf(thread->t_name, sizeof(thread->t_name), "%s_%04d",
> 			 svc->srv_thread_name, thread->t_id);
> 	}
> 
> 	CDEBUG(D_RPCTRACE, "starting thread '%s'\n", thread->t_name);
>-	rc = PTR_ERR(kthread_run(ptlrpc_main, thread, thread->t_name));
>+	rc = PTR_ERR(kthread_run(ptlrpc_main, thread, "%s", thread->t_name));
> 	if (IS_ERR_VALUE(rc)) {
> 		CERROR("cannot start thread '%s': rc %d\n",
> 		       thread->t_name, rc);
>-- 
>1.7.9.5
>
>
>-- 
>Kees Cook
>Chrome OS Security
>


Cheers, Andreas
-- 
Andreas Dilger

Lustre Software Architect
Intel High Performance Data Division


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ