netdev - Re: [PATCH v7 5/8] mm/util: Fix possible race condition in kstrdup()

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CALOAHbBHV_xB88AD8azVXZQzdowLtU6EHewFGUtPHQE9K6GQ_Q@mail.gmail.com>
Date: Fri, 27 Sep 2024 16:57:26 +0800
From: Yafang Shao <laoar.shao@...il.com>
To: Andy Shevchenko <andy.shevchenko@...il.com>
Cc: akpm@...ux-foundation.org, torvalds@...ux-foundation.org, alx@...nel.org, 
	justinstitt@...gle.com, ebiederm@...ssion.com, alexei.starovoitov@...il.com, 
	rostedt@...dmis.org, catalin.marinas@....com, 
	penguin-kernel@...ove.sakura.ne.jp, linux-mm@...ck.org, 
	linux-fsdevel@...r.kernel.org, linux-trace-kernel@...r.kernel.org, 
	audit@...r.kernel.org, linux-security-module@...r.kernel.org, 
	selinux@...r.kernel.org, bpf@...r.kernel.org, netdev@...r.kernel.org, 
	dri-devel@...ts.freedesktop.org
Subject: Re: [PATCH v7 5/8] mm/util: Fix possible race condition in kstrdup()

On Fri, Sep 27, 2024 at 1:35 AM Andy Shevchenko
<andy.shevchenko@...il.com> wrote:
>
> On Thu, Sep 26, 2024 at 7:44 PM Yafang Shao <laoar.shao@...il.com> wrote:
> >
> > In kstrdup(), it is critical to ensure that the dest string is always
> > NUL-terminated. However, potential race condidtion can occur between a
>
> condition
>
> > writer and a reader.
> >
> > Consider the following scenario involving task->comm:
> >
> >     reader                    writer
> >
> >   len = strlen(s) + 1;
> >                              strlcpy(tsk->comm, buf, sizeof(tsk->comm));
> >   memcpy(buf, s, len);
> >
> > In this case, there is a race condition between the reader and the
> > writer. The reader calculate the length of the string `s` based on the
>
> calculates
>
> > old value of task->comm. However, during the memcpy(), the string `s`
> > might be updated by the writer to a new value of task->comm.
> >
> > If the new task->comm is larger than the old one, the `buf` might not be
> > NUL-terminated. This can lead to undefined behavior and potential
> > security vulnerabilities.
> >
> > Let's fix it by explicitly adding a NUL-terminator.
>
> memcpy() is not atomic AFAIK, meaning that the new string can be also
> shorter and when memcpy() already copied past the new NUL. I would
> amend the explanation to include this as well.
>
> ...
>
> > +               /* During memcpy(), the string might be updated to a new value,
> > +                * which could be longer than the string when strlen() is
> > +                * called. Therefore, we need to add a null termimator.
>
> /*
>  * The wrong comment style. Besides that a typo
>  * in the word 'terminator'. Please, run codespell on your changes.
>  * Also use the same form: NUL-terminator when you are talking
>  * about '\0' and not NULL.
>  */

Thank you for pointing out these errors and for recommending the use
of codespell.
Will fix them in the next version.

-- 
Regards
Yafang