BPF Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] um: Fix some error handling in uml_vector_user_bpf()
@ 2020-01-24 10:14 Dan Carpenter
  2020-01-24 12:52 ` Anton Ivanov
  0 siblings, 1 reply; 8+ messages in thread
From: Dan Carpenter @ 2020-01-24 10:14 UTC (permalink / raw)
  To: Jeff Dike, Anton Ivanov
  Cc: Richard Weinberger, Alexei Starovoitov, Daniel Borkmann,
	Martin KaFai Lau, Song Liu, Yonghong Song, Andrii Nakryiko,
	Alex Dewar, linux-um, bpf, kernel-janitors

1) The uml_vector_user_bpf() returns pointers so it should return NULL
   instead of false.
2) If the "bpf_prog" allocation failed, it would have eventually lead to
   a crash.  We can't succeed after the error happens so it should just
   return.

Fixes: 9807019a62dc ("um: Loadable BPF "Firmware" for vector drivers")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
 arch/um/drivers/vector_user.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/um/drivers/vector_user.c b/arch/um/drivers/vector_user.c
index ddcd917be0af..88483f5b034c 100644
--- a/arch/um/drivers/vector_user.c
+++ b/arch/um/drivers/vector_user.c
@@ -732,13 +732,13 @@ void *uml_vector_user_bpf(char *filename)
 
 	if (stat(filename, &statbuf) < 0) {
 		printk(KERN_ERR "Error %d reading bpf file", -errno);
-		return false;
+		return NULL;
 	}
 	bpf_prog = uml_kmalloc(sizeof(struct sock_fprog), UM_GFP_KERNEL);
-	if (bpf_prog != NULL) {
-		bpf_prog->len = statbuf.st_size / sizeof(struct sock_filter);
-		bpf_prog->filter = NULL;
-	}
+	if (!pfg_prog)
+		return NULL;
+	bpf_prog->len = statbuf.st_size / sizeof(struct sock_filter);
+	bpf_prog->filter = NULL;
 	ffd = os_open_file(filename, of_read(OPENFLAGS()), 0);
 	if (ffd < 0) {
 		printk(KERN_ERR "Error %d opening bpf file", -errno);
-- 
2.11.0


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

* Re: [PATCH] um: Fix some error handling in uml_vector_user_bpf()
  2020-01-24 10:14 [PATCH] um: Fix some error handling in uml_vector_user_bpf() Dan Carpenter
@ 2020-01-24 12:52 ` Anton Ivanov
  2020-01-24 16:44   ` Dan Carpenter
  0 siblings, 1 reply; 8+ messages in thread
From: Anton Ivanov @ 2020-01-24 12:52 UTC (permalink / raw)
  To: Dan Carpenter, Jeff Dike
  Cc: Song Liu, Daniel Borkmann, kernel-janitors, Richard Weinberger,
	linux-um, Alexei Starovoitov, Alex Dewar, Yonghong Song, bpf,
	Andrii Nakryiko, Martin KaFai Lau



On 24/01/2020 10:14, Dan Carpenter wrote:
> 1) The uml_vector_user_bpf() returns pointers so it should return NULL
>     instead of false.
> 2) If the "bpf_prog" allocation failed, it would have eventually lead to
>     a crash.  We can't succeed after the error happens so it should just
>     return.
> 
> Fixes: 9807019a62dc ("um: Loadable BPF "Firmware" for vector drivers")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
>   arch/um/drivers/vector_user.c | 10 +++++-----
>   1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/um/drivers/vector_user.c b/arch/um/drivers/vector_user.c
> index ddcd917be0af..88483f5b034c 100644
> --- a/arch/um/drivers/vector_user.c
> +++ b/arch/um/drivers/vector_user.c
> @@ -732,13 +732,13 @@ void *uml_vector_user_bpf(char *filename)
>   
>   	if (stat(filename, &statbuf) < 0) {
>   		printk(KERN_ERR "Error %d reading bpf file", -errno);
> -		return false;
> +		return NULL;

I will sort this one out, thanks for noticing.

>   	}
>   	bpf_prog = uml_kmalloc(sizeof(struct sock_fprog), UM_GFP_KERNEL);
> -	if (bpf_prog != NULL) {
> -		bpf_prog->len = statbuf.st_size / sizeof(struct sock_filter);
> -		bpf_prog->filter = NULL;
> -	}
> +	if (!pfg_prog)

^^^^^ ?

> +		return NULL;
> +	bpf_prog->len = statbuf.st_size / sizeof(struct sock_filter);
> +	bpf_prog->filter = NULL;
>   	ffd = os_open_file(filename, of_read(OPENFLAGS()), 0);
>   	if (ffd < 0) {
>   		printk(KERN_ERR "Error %d opening bpf file", -errno);
> 

-- 
Anton R. Ivanov
Cambridgegreys Limited. Registered in England. Company Number 10273661
https://www.cambridgegreys.com/

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

* Re: [PATCH] um: Fix some error handling in uml_vector_user_bpf()
  2020-01-24 12:52 ` Anton Ivanov
@ 2020-01-24 16:44   ` Dan Carpenter
  2020-01-24 16:48     ` Anton Ivanov
  0 siblings, 1 reply; 8+ messages in thread
From: Dan Carpenter @ 2020-01-24 16:44 UTC (permalink / raw)
  To: Anton Ivanov
  Cc: Jeff Dike, Song Liu, Daniel Borkmann, kernel-janitors,
	Richard Weinberger, linux-um, Alexei Starovoitov, Alex Dewar,
	Yonghong Song, bpf, Andrii Nakryiko, Martin KaFai Lau

On Fri, Jan 24, 2020 at 12:52:18PM +0000, Anton Ivanov wrote:
> 
> 
> On 24/01/2020 10:14, Dan Carpenter wrote:
> > 1) The uml_vector_user_bpf() returns pointers so it should return NULL
> >     instead of false.
> > 2) If the "bpf_prog" allocation failed, it would have eventually lead to
> >     a crash.  We can't succeed after the error happens so it should just
> >     return.
> > 
> > Fixes: 9807019a62dc ("um: Loadable BPF "Firmware" for vector drivers")
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > ---
> >   arch/um/drivers/vector_user.c | 10 +++++-----
> >   1 file changed, 5 insertions(+), 5 deletions(-)
> > 
> > diff --git a/arch/um/drivers/vector_user.c b/arch/um/drivers/vector_user.c
> > index ddcd917be0af..88483f5b034c 100644
> > --- a/arch/um/drivers/vector_user.c
> > +++ b/arch/um/drivers/vector_user.c
> > @@ -732,13 +732,13 @@ void *uml_vector_user_bpf(char *filename)
> >   	if (stat(filename, &statbuf) < 0) {
> >   		printk(KERN_ERR "Error %d reading bpf file", -errno);
> > -		return false;
> > +		return NULL;
> 
> I will sort this one out, thanks for noticing.
> 
> >   	}
> >   	bpf_prog = uml_kmalloc(sizeof(struct sock_fprog), UM_GFP_KERNEL);
> > -	if (bpf_prog != NULL) {
> > -		bpf_prog->len = statbuf.st_size / sizeof(struct sock_filter);
> > -		bpf_prog->filter = NULL;
> > -	}
> > +	if (!pfg_prog)
> 
> ^^^^^ ?

If we don't return here it leads to a NULL dereference.

regards,
dan carpenter


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

* Re: [PATCH] um: Fix some error handling in uml_vector_user_bpf()
  2020-01-24 16:44   ` Dan Carpenter
@ 2020-01-24 16:48     ` Anton Ivanov
  2020-01-25  6:01       ` Dan Carpenter
  2020-01-28 15:10       ` [PATCH] um: Dan Carpenter
  0 siblings, 2 replies; 8+ messages in thread
From: Anton Ivanov @ 2020-01-24 16:48 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: kernel-janitors, Daniel Borkmann, Richard Weinberger, Jeff Dike,
	linux-um, Song Liu, Alexei Starovoitov, Yonghong Song,
	Alex Dewar, bpf, Andrii Nakryiko, Martin KaFai Lau



On 24/01/2020 16:44, Dan Carpenter wrote:
> On Fri, Jan 24, 2020 at 12:52:18PM +0000, Anton Ivanov wrote:
>>
>>
>> On 24/01/2020 10:14, Dan Carpenter wrote:
>>> 1) The uml_vector_user_bpf() returns pointers so it should return NULL
>>>      instead of false.
>>> 2) If the "bpf_prog" allocation failed, it would have eventually lead to
>>>      a crash.  We can't succeed after the error happens so it should just
>>>      return.
>>>
>>> Fixes: 9807019a62dc ("um: Loadable BPF "Firmware" for vector drivers")
>>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>>> ---
>>>    arch/um/drivers/vector_user.c | 10 +++++-----
>>>    1 file changed, 5 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/arch/um/drivers/vector_user.c b/arch/um/drivers/vector_user.c
>>> index ddcd917be0af..88483f5b034c 100644
>>> --- a/arch/um/drivers/vector_user.c
>>> +++ b/arch/um/drivers/vector_user.c
>>> @@ -732,13 +732,13 @@ void *uml_vector_user_bpf(char *filename)
>>>    	if (stat(filename, &statbuf) < 0) {
>>>    		printk(KERN_ERR "Error %d reading bpf file", -errno);
>>> -		return false;
>>> +		return NULL;
>>
>> I will sort this one out, thanks for noticing.
>>
>>>    	}
>>>    	bpf_prog = uml_kmalloc(sizeof(struct sock_fprog), UM_GFP_KERNEL);
>>> -	if (bpf_prog != NULL) {
>>> -		bpf_prog->len = statbuf.st_size / sizeof(struct sock_filter);
>>> -		bpf_prog->filter = NULL;
>>> -	}
>>> +	if (!pfg_prog)
>>
>> ^^^^^ ?
> 
> If we don't return here it leads to a NULL dereference.

It says pfg_prog

I cannot find this identifier :)

> 
> regards,
> dan carpenter
> 
> 
> _______________________________________________
> linux-um mailing list
> linux-um@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-um
> 

-- 
Anton R. Ivanov
Cambridgegreys Limited. Registered in England. Company Number 10273661
https://www.cambridgegreys.com/

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

* Re: [PATCH] um: Fix some error handling in uml_vector_user_bpf()
  2020-01-24 16:48     ` Anton Ivanov
@ 2020-01-25  6:01       ` Dan Carpenter
  2020-01-28 15:10       ` [PATCH] um: Dan Carpenter
  1 sibling, 0 replies; 8+ messages in thread
From: Dan Carpenter @ 2020-01-25  6:01 UTC (permalink / raw)
  To: Anton Ivanov
  Cc: kernel-janitors, Daniel Borkmann, Richard Weinberger, Jeff Dike,
	linux-um, Song Liu, Alexei Starovoitov, Yonghong Song,
	Alex Dewar, bpf, Andrii Nakryiko, Martin KaFai Lau

On Fri, Jan 24, 2020 at 04:48:03PM +0000, Anton Ivanov wrote:
> 
> 
> On 24/01/2020 16:44, Dan Carpenter wrote:
> > On Fri, Jan 24, 2020 at 12:52:18PM +0000, Anton Ivanov wrote:
> > > 
> > > 
> > > On 24/01/2020 10:14, Dan Carpenter wrote:
> > > > 1) The uml_vector_user_bpf() returns pointers so it should return NULL
> > > >      instead of false.
> > > > 2) If the "bpf_prog" allocation failed, it would have eventually lead to
> > > >      a crash.  We can't succeed after the error happens so it should just
> > > >      return.
> > > > 
> > > > Fixes: 9807019a62dc ("um: Loadable BPF "Firmware" for vector drivers")
> > > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > > > ---
> > > >    arch/um/drivers/vector_user.c | 10 +++++-----
> > > >    1 file changed, 5 insertions(+), 5 deletions(-)
> > > > 
> > > > diff --git a/arch/um/drivers/vector_user.c b/arch/um/drivers/vector_user.c
> > > > index ddcd917be0af..88483f5b034c 100644
> > > > --- a/arch/um/drivers/vector_user.c
> > > > +++ b/arch/um/drivers/vector_user.c
> > > > @@ -732,13 +732,13 @@ void *uml_vector_user_bpf(char *filename)
> > > >    	if (stat(filename, &statbuf) < 0) {
> > > >    		printk(KERN_ERR "Error %d reading bpf file", -errno);
> > > > -		return false;
> > > > +		return NULL;
> > > 
> > > I will sort this one out, thanks for noticing.
> > > 
> > > >    	}
> > > >    	bpf_prog = uml_kmalloc(sizeof(struct sock_fprog), UM_GFP_KERNEL);
> > > > -	if (bpf_prog != NULL) {
> > > > -		bpf_prog->len = statbuf.st_size / sizeof(struct sock_filter);
> > > > -		bpf_prog->filter = NULL;
> > > > -	}
> > > > +	if (!pfg_prog)
> > > 
> > > ^^^^^ ?
> > 
> > If we don't return here it leads to a NULL dereference.
> 
> It says pfg_prog
> 
> I cannot find this identifier :)
> 

Oh wow...  That's very embarrassing.  My QC scripts do compile these
as part of the process.  But this wasn't a in of my allmodconfig
and when I do "make arch/um/drivers/vector_user.o", it just silently
returns without printing anything.  I didn't notice that it hadn't
built.

Even "make V=2 arch/um/drivers/vector_user.o" doesn't generate output.

I will resend the patch (on Monday though).

regards,
dan carpenter

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

* [PATCH] um:
  2020-01-24 16:48     ` Anton Ivanov
  2020-01-25  6:01       ` Dan Carpenter
@ 2020-01-28 15:10       ` Dan Carpenter
  2020-01-28 15:27         ` [PATCH v3] um: Fix some error handling in uml_vector_user_bpf() Dan Carpenter
  2020-02-10 14:17         ` Anton Ivanov
  1 sibling, 2 replies; 8+ messages in thread
From: Dan Carpenter @ 2020-01-28 15:10 UTC (permalink / raw)
  To: Jeff Dike, Anton Ivanov
  Cc: Richard Weinberger, Alexei Starovoitov, Daniel Borkmann,
	Martin KaFai Lau, Song Liu, Yonghong Song, Andrii Nakryiko,
	Alex Dewar, linux-um, bpf, kernel-janitors

1) The uml_vector_user_bpf() returns pointers so it should return NULL
   instead of false.
2) If the "bpf_prog" allocation failed, it would have eventually lead to
   a crash.  We can't succeed after the error happens so it should just
   return.

Fixes: 9807019a62dc ("um: Loadable BPF "Firmware" for vector drivers")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
v2: The first version broke the build.  Shame upon me.

 arch/um/drivers/vector_user.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/arch/um/drivers/vector_user.c b/arch/um/drivers/vector_user.c
index ddcd917be0af..1403cbadf92b 100644
--- a/arch/um/drivers/vector_user.c
+++ b/arch/um/drivers/vector_user.c
@@ -732,13 +732,14 @@ void *uml_vector_user_bpf(char *filename)
 
 	if (stat(filename, &statbuf) < 0) {
 		printk(KERN_ERR "Error %d reading bpf file", -errno);
-		return false;
+		return NULL;
 	}
 	bpf_prog = uml_kmalloc(sizeof(struct sock_fprog), UM_GFP_KERNEL);
-	if (bpf_prog != NULL) {
-		bpf_prog->len = statbuf.st_size / sizeof(struct sock_filter);
-		bpf_prog->filter = NULL;
-	}
+	if (bpf_prog == NULL)
+		return NULL;
+	bpf_prog->len = statbuf.st_size / sizeof(struct sock_filter);
+	bpf_prog->filter = NULL;
+
 	ffd = os_open_file(filename, of_read(OPENFLAGS()), 0);
 	if (ffd < 0) {
 		printk(KERN_ERR "Error %d opening bpf file", -errno);
-- 
2.11.0


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

* [PATCH v3] um: Fix some error handling in uml_vector_user_bpf()
  2020-01-28 15:10       ` [PATCH] um: Dan Carpenter
@ 2020-01-28 15:27         ` Dan Carpenter
  2020-02-10 14:17         ` Anton Ivanov
  1 sibling, 0 replies; 8+ messages in thread
From: Dan Carpenter @ 2020-01-28 15:27 UTC (permalink / raw)
  To: Jeff Dike, Anton Ivanov
  Cc: Richard Weinberger, Alexei Starovoitov, Daniel Borkmann,
	Martin KaFai Lau, Song Liu, Yonghong Song, Andrii Nakryiko,
	Alex Dewar, linux-um, bpf, kernel-janitors

1) The uml_vector_user_bpf() returns pointers so it should return NULL
   instead of false.
2) If the "bpf_prog" allocation failed, it would have eventually lead to
   a crash.  We can't succeed after the error happens so it should just
   return.

Fixes: 9807019a62dc ("um: Loadable BPF "Firmware" for vector drivers")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
v3: Fix screwed up subject.  Sorry.  Not my most shining hour.
v2: The first version broke the build.  Shame upon me.

 arch/um/drivers/vector_user.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/arch/um/drivers/vector_user.c b/arch/um/drivers/vector_user.c
index ddcd917be0af..1403cbadf92b 100644
--- a/arch/um/drivers/vector_user.c
+++ b/arch/um/drivers/vector_user.c
@@ -732,13 +732,14 @@ void *uml_vector_user_bpf(char *filename)
 
 	if (stat(filename, &statbuf) < 0) {
 		printk(KERN_ERR "Error %d reading bpf file", -errno);
-		return false;
+		return NULL;
 	}
 	bpf_prog = uml_kmalloc(sizeof(struct sock_fprog), UM_GFP_KERNEL);
-	if (bpf_prog != NULL) {
-		bpf_prog->len = statbuf.st_size / sizeof(struct sock_filter);
-		bpf_prog->filter = NULL;
-	}
+	if (bpf_prog == NULL)
+		return NULL;
+	bpf_prog->len = statbuf.st_size / sizeof(struct sock_filter);
+	bpf_prog->filter = NULL;
+
 	ffd = os_open_file(filename, of_read(OPENFLAGS()), 0);
 	if (ffd < 0) {
 		printk(KERN_ERR "Error %d opening bpf file", -errno);
-- 
2.11.0


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

* Re: [PATCH v3] um: Fix some error handling in uml_vector_user_bpf()
  2020-01-28 15:10       ` [PATCH] um: Dan Carpenter
  2020-01-28 15:27         ` [PATCH v3] um: Fix some error handling in uml_vector_user_bpf() Dan Carpenter
@ 2020-02-10 14:17         ` Anton Ivanov
  1 sibling, 0 replies; 8+ messages in thread
From: Anton Ivanov @ 2020-02-10 14:17 UTC (permalink / raw)
  To: Dan Carpenter, Jeff Dike
  Cc: Richard Weinberger, Alexei Starovoitov, Daniel Borkmann,
	Martin KaFai Lau, Song Liu, Yonghong Song, Andrii Nakryiko,
	Alex Dewar, linux-um, bpf, kernel-janitors



On 28/01/2020 15:27, Dan Carpenter wrote:
> 1) The uml_vector_user_bpf() returns pointers so it should return NULL
>     instead of false.
> 2) If the "bpf_prog" allocation failed, it would have eventually lead to
>     a crash.  We can't succeed after the error happens so it should just
>     return.
> 
> Fixes: 9807019a62dc ("um: Loadable BPF "Firmware" for vector drivers")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> v3: Fix screwed up subject.  Sorry.  Not my most shining hour.
> v2: The first version broke the build.  Shame upon me.
> 
>   arch/um/drivers/vector_user.c | 11 ++++++-----
>   1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/um/drivers/vector_user.c b/arch/um/drivers/vector_user.c
> index ddcd917be0af..1403cbadf92b 100644
> --- a/arch/um/drivers/vector_user.c
> +++ b/arch/um/drivers/vector_user.c
> @@ -732,13 +732,14 @@ void *uml_vector_user_bpf(char *filename)
>   
>   	if (stat(filename, &statbuf) < 0) {
>   		printk(KERN_ERR "Error %d reading bpf file", -errno);
> -		return false;
> +		return NULL;
>   	}
>   	bpf_prog = uml_kmalloc(sizeof(struct sock_fprog), UM_GFP_KERNEL);
> -	if (bpf_prog != NULL) {
> -		bpf_prog->len = statbuf.st_size / sizeof(struct sock_filter);
> -		bpf_prog->filter = NULL;
> -	}
> +	if (bpf_prog == NULL)
> +		return NULL;
> +	bpf_prog->len = statbuf.st_size / sizeof(struct sock_filter);
> +	bpf_prog->filter = NULL;
> +
>   	ffd = os_open_file(filename, of_read(OPENFLAGS()), 0);
>   	if (ffd < 0) {
>   		printk(KERN_ERR "Error %d opening bpf file", -errno);
> 

Acked-by: Anton Ivanov <anton.ivanov@cambridgegreys.com>

-- 
Anton R. Ivanov
Cambridgegreys Limited. Registered in England. Company Number 10273661
https://www.cambridgegreys.com/

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

end of thread, back to index

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-24 10:14 [PATCH] um: Fix some error handling in uml_vector_user_bpf() Dan Carpenter
2020-01-24 12:52 ` Anton Ivanov
2020-01-24 16:44   ` Dan Carpenter
2020-01-24 16:48     ` Anton Ivanov
2020-01-25  6:01       ` Dan Carpenter
2020-01-28 15:10       ` [PATCH] um: Dan Carpenter
2020-01-28 15:27         ` [PATCH v3] um: Fix some error handling in uml_vector_user_bpf() Dan Carpenter
2020-02-10 14:17         ` Anton Ivanov

BPF Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/bpf/0 bpf/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 bpf bpf/ https://lore.kernel.org/bpf \
		bpf@vger.kernel.org
	public-inbox-index bpf

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.bpf


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git