| Message ID | 20200124101450.jxfzsh6sz7v324hv@kili.mountain |
|---|---|
| State | Superseded |
| Headers | show |
| Series | um: Fix some error handling in uml_vector_user_bpf() | expand |
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); >
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
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 >
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
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);
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(-)