linux-modules.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/2] Fix a couple of potential problem (static analysis)
@ 2021-06-08  6:28 Yauheni Kaliuta
  2021-06-08  6:29 ` [PATCH 1/2] libkmod-module: check "new_from_name" return value in get_builtin Yauheni Kaliuta
  0 siblings, 1 reply; 8+ messages in thread
From: Yauheni Kaliuta @ 2021-06-08  6:28 UTC (permalink / raw)
  To: linux-modules; +Cc: lucas.de.marchi, Yauheni Kaliuta


Yauheni Kaliuta (2):
  libkmod-module: check "new_from_name" return value in get_builtin
  libkmod-builtin: consider final NIL in name length check

 libkmod/libkmod-builtin.c | 2 +-
 libkmod/libkmod-module.c  | 5 ++++-
 2 files changed, 5 insertions(+), 2 deletions(-)

-- 
2.31.1


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 1/2] libkmod-module: check "new_from_name" return value in get_builtin
  2021-06-08  6:28 [PATCH 0/2] Fix a couple of potential problem (static analysis) Yauheni Kaliuta
@ 2021-06-08  6:29 ` Yauheni Kaliuta
  2021-06-08  6:29   ` [PATCH 2/2] libkmod-builtin: consider final NIL in name length check Yauheni Kaliuta
  2021-06-09 17:22   ` [PATCH 1/2] libkmod-module: check "new_from_name" return value in get_builtin Lucas De Marchi
  0 siblings, 2 replies; 8+ messages in thread
From: Yauheni Kaliuta @ 2021-06-08  6:29 UTC (permalink / raw)
  To: linux-modules; +Cc: lucas.de.marchi, Yauheni Kaliuta

kmod_module_new_from_name() may fail and return error value. It is
handled properly across the code, but in this particular place the
check is missing.

Signed-off-by: Yauheni Kaliuta <ykaliuta@redhat.com>
---
 libkmod/libkmod-module.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/libkmod/libkmod-module.c b/libkmod/libkmod-module.c
index b6320cc87e80..6e0ff1a99604 100644
--- a/libkmod/libkmod-module.c
+++ b/libkmod/libkmod-module.c
@@ -2912,7 +2912,10 @@ int kmod_module_get_builtin(struct kmod_ctx *ctx, struct kmod_list **list)
 			goto fail;
 		}
 
-		kmod_module_new_from_name(ctx, modname, &mod);
+		err = kmod_module_new_from_name(ctx, modname, &mod);
+		if (err < 0)
+			goto fail;
+
 		kmod_module_set_builtin(mod, true);
 
 		*list = kmod_list_append(*list, mod);
-- 
2.31.1


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 2/2] libkmod-builtin: consider final NIL in name length check
  2021-06-08  6:29 ` [PATCH 1/2] libkmod-module: check "new_from_name" return value in get_builtin Yauheni Kaliuta
@ 2021-06-08  6:29   ` Yauheni Kaliuta
  2021-06-09 17:10     ` Lucas De Marchi
  2021-06-09 17:22   ` [PATCH 1/2] libkmod-module: check "new_from_name" return value in get_builtin Lucas De Marchi
  1 sibling, 1 reply; 8+ messages in thread
From: Yauheni Kaliuta @ 2021-06-08  6:29 UTC (permalink / raw)
  To: linux-modules; +Cc: lucas.de.marchi, Yauheni Kaliuta

There is potential buffer overrun in kmod_builtin_iter_get_modname()
for the name of length PATH_MAX. The required buffer size is
PATH_MAX, so `modname[len] = '\0'` with len == PATH_MAX will write
right beyond the buffer.

Check the length against PATH_MAX - 1.

Signed-off-by: Yauheni Kaliuta <ykaliuta@redhat.com>
---
 libkmod/libkmod-builtin.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libkmod/libkmod-builtin.c b/libkmod/libkmod-builtin.c
index a002cb5ee2c6..3d4d77ab29b3 100644
--- a/libkmod/libkmod-builtin.c
+++ b/libkmod/libkmod-builtin.c
@@ -246,7 +246,7 @@ bool kmod_builtin_iter_get_modname(struct kmod_builtin_iter *iter,
 
 	len = dot - line;
 
-	if (len >= PATH_MAX) {
+	if (len >= PATH_MAX - 1) {
 		sv_errno = ENAMETOOLONG;
 		goto fail;
 	}
-- 
2.31.1


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/2] libkmod-builtin: consider final NIL in name length check
  2021-06-08  6:29   ` [PATCH 2/2] libkmod-builtin: consider final NIL in name length check Yauheni Kaliuta
@ 2021-06-09 17:10     ` Lucas De Marchi
  2021-06-09 17:18       ` Lucas De Marchi
  2021-06-22  6:22       ` Yauheni Kaliuta
  0 siblings, 2 replies; 8+ messages in thread
From: Lucas De Marchi @ 2021-06-09 17:10 UTC (permalink / raw)
  To: Yauheni Kaliuta; +Cc: linux-modules, lucas.de.marchi

On Tue, Jun 08, 2021 at 09:29:23AM +0300, Yauheni Kaliuta wrote:
>There is potential buffer overrun in kmod_builtin_iter_get_modname()
>for the name of length PATH_MAX. The required buffer size is
>PATH_MAX, so `modname[len] = '\0'` with len == PATH_MAX will write
>right beyond the buffer.

this doesn't look correct. "with len == PATH_MAX" we will actually
return an error. 

What indeed is happening is truncation: since we are not reserving 1
char for NUL termination, we will truncate the name. If we update the
commit message to state the right reasoning, then we can land this patch.

I don't see any buffer overflow here, but I may be missing something.

thanks
LUcas De Marchi

>
>Check the length against PATH_MAX - 1.
>
>Signed-off-by: Yauheni Kaliuta <ykaliuta@redhat.com>
>---
> libkmod/libkmod-builtin.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/libkmod/libkmod-builtin.c b/libkmod/libkmod-builtin.c
>index a002cb5ee2c6..3d4d77ab29b3 100644
>--- a/libkmod/libkmod-builtin.c
>+++ b/libkmod/libkmod-builtin.c
>@@ -246,7 +246,7 @@ bool kmod_builtin_iter_get_modname(struct kmod_builtin_iter *iter,
>
> 	len = dot - line;
>
>-	if (len >= PATH_MAX) {
>+	if (len >= PATH_MAX - 1) {
> 		sv_errno = ENAMETOOLONG;
> 		goto fail;
> 	}
>-- 
>2.31.1
>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/2] libkmod-builtin: consider final NIL in name length check
  2021-06-09 17:10     ` Lucas De Marchi
@ 2021-06-09 17:18       ` Lucas De Marchi
  2021-06-22  6:31         ` Yauheni Kaliuta
  2021-06-22  6:22       ` Yauheni Kaliuta
  1 sibling, 1 reply; 8+ messages in thread
From: Lucas De Marchi @ 2021-06-09 17:18 UTC (permalink / raw)
  To: Yauheni Kaliuta; +Cc: linux-modules, lucas.de.marchi

On Wed, Jun 09, 2021 at 10:10:53AM -0700, Lucas De Marchi wrote:
>On Tue, Jun 08, 2021 at 09:29:23AM +0300, Yauheni Kaliuta wrote:
>>There is potential buffer overrun in kmod_builtin_iter_get_modname()
>>for the name of length PATH_MAX. The required buffer size is
>>PATH_MAX, so `modname[len] = '\0'` with len == PATH_MAX will write
>>right beyond the buffer.
>
>this doesn't look correct. "with len == PATH_MAX" we will actually
>return an error.
>
>What indeed is happening is truncation: since we are not reserving 1
>char for NUL termination, we will truncate the name. If we update the
>commit message to state the right reasoning, then we can land this patch.
>
>I don't see any buffer overflow here, but I may be missing something.

another thing... what is your git-sendemail setup? This is putting patch
2 as a reply to patch 1 and that breaks b4. See:
https://lore.kernel.org/linux-modules/20210608062923.94017-1-ykaliuta@redhat.com/T/#u

Lucas De Marchi

>
>thanks
>LUcas De Marchi
>
>>
>>Check the length against PATH_MAX - 1.
>>
>>Signed-off-by: Yauheni Kaliuta <ykaliuta@redhat.com>
>>---
>>libkmod/libkmod-builtin.c | 2 +-
>>1 file changed, 1 insertion(+), 1 deletion(-)
>>
>>diff --git a/libkmod/libkmod-builtin.c b/libkmod/libkmod-builtin.c
>>index a002cb5ee2c6..3d4d77ab29b3 100644
>>--- a/libkmod/libkmod-builtin.c
>>+++ b/libkmod/libkmod-builtin.c
>>@@ -246,7 +246,7 @@ bool kmod_builtin_iter_get_modname(struct kmod_builtin_iter *iter,
>>
>>	len = dot - line;
>>
>>-	if (len >= PATH_MAX) {
>>+	if (len >= PATH_MAX - 1) {
>>		sv_errno = ENAMETOOLONG;
>>		goto fail;
>>	}
>>-- 
>>2.31.1
>>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/2] libkmod-module: check "new_from_name" return value in get_builtin
  2021-06-08  6:29 ` [PATCH 1/2] libkmod-module: check "new_from_name" return value in get_builtin Yauheni Kaliuta
  2021-06-08  6:29   ` [PATCH 2/2] libkmod-builtin: consider final NIL in name length check Yauheni Kaliuta
@ 2021-06-09 17:22   ` Lucas De Marchi
  1 sibling, 0 replies; 8+ messages in thread
From: Lucas De Marchi @ 2021-06-09 17:22 UTC (permalink / raw)
  To: Yauheni Kaliuta; +Cc: linux-modules, lucas.de.marchi

On Tue, Jun 08, 2021 at 09:29:22AM +0300, Yauheni Kaliuta wrote:
>kmod_module_new_from_name() may fail and return error value. It is
>handled properly across the code, but in this particular place the
>check is missing.
>
>Signed-off-by: Yauheni Kaliuta <ykaliuta@redhat.com>

applied, thanks

Lucas De Marchi

>---
> libkmod/libkmod-module.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
>diff --git a/libkmod/libkmod-module.c b/libkmod/libkmod-module.c
>index b6320cc87e80..6e0ff1a99604 100644
>--- a/libkmod/libkmod-module.c
>+++ b/libkmod/libkmod-module.c
>@@ -2912,7 +2912,10 @@ int kmod_module_get_builtin(struct kmod_ctx *ctx, struct kmod_list **list)
> 			goto fail;
> 		}
>
>-		kmod_module_new_from_name(ctx, modname, &mod);
>+		err = kmod_module_new_from_name(ctx, modname, &mod);
>+		if (err < 0)
>+			goto fail;
>+
> 		kmod_module_set_builtin(mod, true);
>
> 		*list = kmod_list_append(*list, mod);
>-- 
>2.31.1
>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/2] libkmod-builtin: consider final NIL in name length check
  2021-06-09 17:10     ` Lucas De Marchi
  2021-06-09 17:18       ` Lucas De Marchi
@ 2021-06-22  6:22       ` Yauheni Kaliuta
  1 sibling, 0 replies; 8+ messages in thread
From: Yauheni Kaliuta @ 2021-06-22  6:22 UTC (permalink / raw)
  To: Lucas De Marchi; +Cc: linux-modules, lucas.de.marchi

Hi, Lucas!

My filters missed your replies, sorry :(


On Wed, Jun 9, 2021 at 8:11 PM Lucas De Marchi <lucas.demarchi@intel.com> wrote:
>
> On Tue, Jun 08, 2021 at 09:29:23AM +0300, Yauheni Kaliuta wrote:
> >There is potential buffer overrun in kmod_builtin_iter_get_modname()
> >for the name of length PATH_MAX. The required buffer size is
> >PATH_MAX, so `modname[len] = '\0'` with len == PATH_MAX will write
> >right beyond the buffer.
>
> this doesn't look correct. "with len == PATH_MAX" we will actually
> return an error.
>
> What indeed is happening is truncation: since we are not reserving 1
> char for NUL termination, we will truncate the name. If we update the
> commit message to state the right reasoning, then we can land this patch.
>
> I don't see any buffer overflow here, but I may be missing something.

Yes, you are right. I see the actual overflow is fixed by
1cab02ecf6ee ("libkmod: fix an overflow with wrong modules.builtin.modinfo")

I should have checked some reports more carefully. Please, ignore the patch.

> thanks
> LUcas De Marchi
>
> >
> >Check the length against PATH_MAX - 1.
> >
> >Signed-off-by: Yauheni Kaliuta <ykaliuta@redhat.com>
> >---
> > libkmod/libkmod-builtin.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> >diff --git a/libkmod/libkmod-builtin.c b/libkmod/libkmod-builtin.c
> >index a002cb5ee2c6..3d4d77ab29b3 100644
> >--- a/libkmod/libkmod-builtin.c
> >+++ b/libkmod/libkmod-builtin.c
> >@@ -246,7 +246,7 @@ bool kmod_builtin_iter_get_modname(struct kmod_builtin_iter *iter,
> >
> >       len = dot - line;
> >
> >-      if (len >= PATH_MAX) {
> >+      if (len >= PATH_MAX - 1) {
> >               sv_errno = ENAMETOOLONG;
> >               goto fail;
> >       }
> >--
> >2.31.1
> >
>


--
WBR, Yauheni


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/2] libkmod-builtin: consider final NIL in name length check
  2021-06-09 17:18       ` Lucas De Marchi
@ 2021-06-22  6:31         ` Yauheni Kaliuta
  0 siblings, 0 replies; 8+ messages in thread
From: Yauheni Kaliuta @ 2021-06-22  6:31 UTC (permalink / raw)
  To: Lucas De Marchi; +Cc: linux-modules, lucas.de.marchi

On Wed, Jun 9, 2021 at 8:19 PM Lucas De Marchi <lucas.demarchi@intel.com> wrote:
>
> On Wed, Jun 09, 2021 at 10:10:53AM -0700, Lucas De Marchi wrote:
> >On Tue, Jun 08, 2021 at 09:29:23AM +0300, Yauheni Kaliuta wrote:
> >>There is potential buffer overrun in kmod_builtin_iter_get_modname()
> >>for the name of length PATH_MAX. The required buffer size is
> >>PATH_MAX, so `modname[len] = '\0'` with len == PATH_MAX will write
> >>right beyond the buffer.
> >
> >this doesn't look correct. "with len == PATH_MAX" we will actually
> >return an error.
> >
> >What indeed is happening is truncation: since we are not reserving 1
> >char for NUL termination, we will truncate the name. If we update the
> >commit message to state the right reasoning, then we can land this patch.
> >
> >I don't see any buffer overflow here, but I may be missing something.
>
> another thing... what is your git-sendemail setup? This is putting patch
> 2 as a reply to patch 1 and that breaks b4. See:
> https://lore.kernel.org/linux-modules/20210608062923.94017-1-ykaliuta@redhat.com/T/#u

Thanks, yes. I have thread = true, but send it after the cover letter with

git send-email --to linux-modules@vger.kernel.org --cc
lucas.de.marchi@gmail.com
--in-reply-to=20210608062859.93959-1-ykaliuta@redhat.com
0001-libkmod-module-check-new_from_name-return-value-in-g.patch
0002-libkmod-builtin-consider-final-NIL-in-name-length-ch.patch

So, the right way is to send all together, or disable 'thread'.

>
> Lucas De Marchi
>
> >
> >thanks
> >LUcas De Marchi
> >
> >>
> >>Check the length against PATH_MAX - 1.
> >>
> >>Signed-off-by: Yauheni Kaliuta <ykaliuta@redhat.com>
> >>---
> >>libkmod/libkmod-builtin.c | 2 +-
> >>1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >>diff --git a/libkmod/libkmod-builtin.c b/libkmod/libkmod-builtin.c
> >>index a002cb5ee2c6..3d4d77ab29b3 100644
> >>--- a/libkmod/libkmod-builtin.c
> >>+++ b/libkmod/libkmod-builtin.c
> >>@@ -246,7 +246,7 @@ bool kmod_builtin_iter_get_modname(struct kmod_builtin_iter *iter,
> >>
> >>      len = dot - line;
> >>
> >>-     if (len >= PATH_MAX) {
> >>+     if (len >= PATH_MAX - 1) {
> >>              sv_errno = ENAMETOOLONG;
> >>              goto fail;
> >>      }
> >>--
> >>2.31.1
> >>
>


-- 
WBR, Yauheni


^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2021-06-22  6:31 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-08  6:28 [PATCH 0/2] Fix a couple of potential problem (static analysis) Yauheni Kaliuta
2021-06-08  6:29 ` [PATCH 1/2] libkmod-module: check "new_from_name" return value in get_builtin Yauheni Kaliuta
2021-06-08  6:29   ` [PATCH 2/2] libkmod-builtin: consider final NIL in name length check Yauheni Kaliuta
2021-06-09 17:10     ` Lucas De Marchi
2021-06-09 17:18       ` Lucas De Marchi
2021-06-22  6:31         ` Yauheni Kaliuta
2021-06-22  6:22       ` Yauheni Kaliuta
2021-06-09 17:22   ` [PATCH 1/2] libkmod-module: check "new_from_name" return value in get_builtin Lucas De Marchi

This is a public inbox, see mirroring instructions
on how to clone and mirror all data and code used for this inbox