From: "Christian König" <christian.koenig@amd.com>
To: "T.J. Mercier" <tjmercier@google.com>, Kees Cook <keescook@chromium.org>
Cc: Sumit Semwal <sumit.semwal@linaro.org>,
linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org,
Azeem Shaikh <azeemshaikh38@gmail.com>,
linaro-mm-sig@lists.linaro.org, linux-hardening@vger.kernel.org,
linux-media@vger.kernel.org
Subject: Re: [PATCH] dma-buf: Replace strlcpy() with strscpy()
Date: Mon, 20 Nov 2023 10:59:45 +0100 [thread overview]
Message-ID: <4ff92772-9194-42db-b8af-8024e1fdf59f@amd.com> (raw)
In-Reply-To: <CABdmKX1oNw+quAd+ALcgGoz-PPsvy=O6YM4f2_SmP+dQBddzAA@mail.gmail.com>
Am 17.11.23 um 19:50 schrieb T.J. Mercier:
> On Thu, Nov 16, 2023 at 11:14 AM Kees Cook <keescook@chromium.org> wrote:
>> strlcpy() reads the entire source buffer first. This read may exceed
>> the destination size limit. This is both inefficient and can lead
>> to linear read overflows if a source string is not NUL-terminated[1].
>> Additionally, it returns the size of the source string, not the
>> resulting size of the destination string. In an effort to remove strlcpy()
>> completely[2], replace strlcpy() here with strscpy().
>>
>> Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy [1]
>> Link: https://github.com/KSPP/linux/issues/89 [2]
>> Cc: Sumit Semwal <sumit.semwal@linaro.org>
>> Cc: "Christian König" <christian.koenig@amd.com>
>> Cc: Azeem Shaikh <azeemshaikh38@gmail.com>
>> Cc: linux-media@vger.kernel.org
>> Cc: dri-devel@lists.freedesktop.org
>> Cc: linaro-mm-sig@lists.linaro.org
>> Signed-off-by: Kees Cook <keescook@chromium.org>
> Reviewed-by: T.J. Mercier <tjmercier@google.com>
>
> strscpy returns -E2BIG when it truncates / force null-terminates which
> would provide the wrong argument for dynamic_dname, but
> dma_buf_set_name{_user} makes sure we have a null-terminated string of
> the appropriate maximum size in dmabuf->name.
Thanks for that background check, I was about to note that this might
not be a good idea.
Linus pretty clearly stated that he doesn't want to see patches like
that one here, see this article as well. https://lwn.net/Articles/659214/
I think the commit message gives enough reason to merge the patch, so
I'm going to push it to drm-misc-next. But please make sure to triple
check stuff like this before sending.
Thanks,
Christian.
WARNING: multiple messages have this Message-ID (diff)
From: "Christian König" <christian.koenig@amd.com>
To: "T.J. Mercier" <tjmercier@google.com>, Kees Cook <keescook@chromium.org>
Cc: linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org,
Azeem Shaikh <azeemshaikh38@gmail.com>,
linaro-mm-sig@lists.linaro.org, linux-hardening@vger.kernel.org,
Sumit Semwal <sumit.semwal@linaro.org>,
linux-media@vger.kernel.org
Subject: Re: [PATCH] dma-buf: Replace strlcpy() with strscpy()
Date: Mon, 20 Nov 2023 10:59:45 +0100 [thread overview]
Message-ID: <4ff92772-9194-42db-b8af-8024e1fdf59f@amd.com> (raw)
In-Reply-To: <CABdmKX1oNw+quAd+ALcgGoz-PPsvy=O6YM4f2_SmP+dQBddzAA@mail.gmail.com>
Am 17.11.23 um 19:50 schrieb T.J. Mercier:
> On Thu, Nov 16, 2023 at 11:14 AM Kees Cook <keescook@chromium.org> wrote:
>> strlcpy() reads the entire source buffer first. This read may exceed
>> the destination size limit. This is both inefficient and can lead
>> to linear read overflows if a source string is not NUL-terminated[1].
>> Additionally, it returns the size of the source string, not the
>> resulting size of the destination string. In an effort to remove strlcpy()
>> completely[2], replace strlcpy() here with strscpy().
>>
>> Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy [1]
>> Link: https://github.com/KSPP/linux/issues/89 [2]
>> Cc: Sumit Semwal <sumit.semwal@linaro.org>
>> Cc: "Christian König" <christian.koenig@amd.com>
>> Cc: Azeem Shaikh <azeemshaikh38@gmail.com>
>> Cc: linux-media@vger.kernel.org
>> Cc: dri-devel@lists.freedesktop.org
>> Cc: linaro-mm-sig@lists.linaro.org
>> Signed-off-by: Kees Cook <keescook@chromium.org>
> Reviewed-by: T.J. Mercier <tjmercier@google.com>
>
> strscpy returns -E2BIG when it truncates / force null-terminates which
> would provide the wrong argument for dynamic_dname, but
> dma_buf_set_name{_user} makes sure we have a null-terminated string of
> the appropriate maximum size in dmabuf->name.
Thanks for that background check, I was about to note that this might
not be a good idea.
Linus pretty clearly stated that he doesn't want to see patches like
that one here, see this article as well. https://lwn.net/Articles/659214/
I think the commit message gives enough reason to merge the patch, so
I'm going to push it to drm-misc-next. But please make sure to triple
check stuff like this before sending.
Thanks,
Christian.
next prev parent reply other threads:[~2023-11-20 9:59 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-11-16 19:14 [PATCH] dma-buf: Replace strlcpy() with strscpy() Kees Cook
2023-11-16 19:14 ` Kees Cook
2023-11-17 18:50 ` T.J. Mercier
2023-11-17 18:50 ` T.J. Mercier
2023-11-20 9:59 ` Christian König [this message]
2023-11-20 9:59 ` Christian König
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4ff92772-9194-42db-b8af-8024e1fdf59f@amd.com \
--to=christian.koenig@amd.com \
--cc=azeemshaikh38@gmail.com \
--cc=dri-devel@lists.freedesktop.org \
--cc=keescook@chromium.org \
--cc=linaro-mm-sig@lists.linaro.org \
--cc=linux-hardening@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=sumit.semwal@linaro.org \
--cc=tjmercier@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.