* [PATCH] media: meson: Add NULL check after the call to kmalloc() @ 2019-09-04 8:22 Austin Kim 2019-09-04 8:43 ` Rasmus Villemoes 2019-09-04 8:45 ` Greg KH 0 siblings, 2 replies; 5+ messages in thread From: Austin Kim @ 2019-09-04 8:22 UTC (permalink / raw) To: mchehab, khilman Cc: mjourdan, gregkh, linux-media, linux-amlogic, devel, linux-arm-kernel, linux-kernel If the kmalloc() return NULL, the NULL pointer dereference will occur. new_ts->ts = ts; Add exception check after the call to kmalloc() is made. Signed-off-by: Austin Kim <austindh.kim@gmail.com> --- drivers/staging/media/meson/vdec/vdec_helpers.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/drivers/staging/media/meson/vdec/vdec_helpers.c b/drivers/staging/media/meson/vdec/vdec_helpers.c index f16948b..e7e56d5 100644 --- a/drivers/staging/media/meson/vdec/vdec_helpers.c +++ b/drivers/staging/media/meson/vdec/vdec_helpers.c @@ -206,6 +206,10 @@ void amvdec_add_ts_reorder(struct amvdec_session *sess, u64 ts, u32 offset) unsigned long flags; new_ts = kmalloc(sizeof(*new_ts), GFP_KERNEL); + if (!new_ts) { + dev_err(sess->core->dev, "Failed to kmalloc()\n"); + return; + } new_ts->ts = ts; new_ts->offset = offset; -- 2.6.2 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] media: meson: Add NULL check after the call to kmalloc() 2019-09-04 8:22 [PATCH] media: meson: Add NULL check after the call to kmalloc() Austin Kim @ 2019-09-04 8:43 ` Rasmus Villemoes 2019-09-04 22:47 ` Austin Kim 2019-09-04 8:45 ` Greg KH 1 sibling, 1 reply; 5+ messages in thread From: Rasmus Villemoes @ 2019-09-04 8:43 UTC (permalink / raw) To: Austin Kim, mchehab, khilman Cc: mjourdan, gregkh, linux-media, linux-amlogic, devel, linux-arm-kernel, linux-kernel On 04/09/2019 10.22, Austin Kim wrote: > If the kmalloc() return NULL, the NULL pointer dereference will occur. > new_ts->ts = ts; > > Add exception check after the call to kmalloc() is made. > > Signed-off-by: Austin Kim <austindh.kim@gmail.com> > --- > drivers/staging/media/meson/vdec/vdec_helpers.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/staging/media/meson/vdec/vdec_helpers.c b/drivers/staging/media/meson/vdec/vdec_helpers.c > index f16948b..e7e56d5 100644 > --- a/drivers/staging/media/meson/vdec/vdec_helpers.c > +++ b/drivers/staging/media/meson/vdec/vdec_helpers.c > @@ -206,6 +206,10 @@ void amvdec_add_ts_reorder(struct amvdec_session *sess, u64 ts, u32 offset) > unsigned long flags; > > new_ts = kmalloc(sizeof(*new_ts), GFP_KERNEL); > + if (!new_ts) { > + dev_err(sess->core->dev, "Failed to kmalloc()\n"); > + return; > + } > new_ts->ts = ts; > new_ts->offset = offset; No need to printk() on error, AFAIK the mm subsystem should already make some noise if an allocation fails. This is not a proper fix - you need to make the function return an error (-ENOMEM) to let the caller know allocation failed, and allow that to propagate the error. There's only one caller, which already seems capable of returning errors (there's an -EAGAIN), so it shouldn't be that hard - though of course one needs to undo what has been done so far. Also, unrelated to the kmalloc() handling: amvdec_add_ts_reorder() could be moved to esparser.c and made static, or at the very least the EXPORT_SYMBOL can be removed since vdec_helpers.o is linked in to the same module as the sole user. That probably goes for all the EXPORT_SYMBOL(amvdec_*). Rasmus ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] media: meson: Add NULL check after the call to kmalloc() 2019-09-04 8:43 ` Rasmus Villemoes @ 2019-09-04 22:47 ` Austin Kim 0 siblings, 0 replies; 5+ messages in thread From: Austin Kim @ 2019-09-04 22:47 UTC (permalink / raw) To: Rasmus Villemoes Cc: mchehab, khilman, mjourdan, gregkh, linux-media, linux-amlogic, devel, linux-arm-kernel, linux-kernel 2019년 9월 4일 (수) 오후 5:43, Rasmus Villemoes <linux@rasmusvillemoes.dk>님이 작성: > > On 04/09/2019 10.22, Austin Kim wrote: > > If the kmalloc() return NULL, the NULL pointer dereference will occur. > > new_ts->ts = ts; > > > > Add exception check after the call to kmalloc() is made. > > > > Signed-off-by: Austin Kim <austindh.kim@gmail.com> > > --- > > drivers/staging/media/meson/vdec/vdec_helpers.c | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/drivers/staging/media/meson/vdec/vdec_helpers.c b/drivers/staging/media/meson/vdec/vdec_helpers.c > > index f16948b..e7e56d5 100644 > > --- a/drivers/staging/media/meson/vdec/vdec_helpers.c > > +++ b/drivers/staging/media/meson/vdec/vdec_helpers.c > > @@ -206,6 +206,10 @@ void amvdec_add_ts_reorder(struct amvdec_session *sess, u64 ts, u32 offset) > > unsigned long flags; > > > > new_ts = kmalloc(sizeof(*new_ts), GFP_KERNEL); > > + if (!new_ts) { > > + dev_err(sess->core->dev, "Failed to kmalloc()\n"); > > + return; > > + } > > new_ts->ts = ts; > > new_ts->offset = offset; > > No need to printk() on error, AFAIK the mm subsystem should already make > some noise if an allocation fails. Thanks for valuable feedback. BTW, if the kmalloc return NULL, mm subsystem throws error log with 100%? If no, please share error signature of kernel. > This is not a proper fix - you need to make the function return an error > (-ENOMEM) to let the caller know allocation failed, and allow that to > propagate the error. There's only one caller, which already seems > capable of returning errors (there's an -EAGAIN), so it shouldn't be > that hard - though of course one needs to undo what has been done so far. > > Also, unrelated to the kmalloc() handling: amvdec_add_ts_reorder() could > be moved to esparser.c and made static, or at the very least the > EXPORT_SYMBOL can be removed since vdec_helpers.o is linked in to the > same module as the sole user. That probably goes for all the > EXPORT_SYMBOL(amvdec_*). I agreed with your feedback. - On memory allocation failure, it should have returned (-ENOMEM) rather than 'return' statement. - The call to amvdec_add_ts_reorder() is only made by esparser_queue(). Let me resend the patch if necessary. Thanks, Austin Kim > > Rasmus ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] media: meson: Add NULL check after the call to kmalloc() 2019-09-04 8:22 [PATCH] media: meson: Add NULL check after the call to kmalloc() Austin Kim 2019-09-04 8:43 ` Rasmus Villemoes @ 2019-09-04 8:45 ` Greg KH 2019-09-04 22:39 ` Austin Kim 1 sibling, 1 reply; 5+ messages in thread From: Greg KH @ 2019-09-04 8:45 UTC (permalink / raw) To: Austin Kim Cc: mchehab, khilman, mjourdan, devel, linux-kernel, linux-amlogic, linux-arm-kernel, linux-media On Wed, Sep 04, 2019 at 05:22:32PM +0900, Austin Kim wrote: > If the kmalloc() return NULL, the NULL pointer dereference will occur. > new_ts->ts = ts; > > Add exception check after the call to kmalloc() is made. > > Signed-off-by: Austin Kim <austindh.kim@gmail.com> > --- > drivers/staging/media/meson/vdec/vdec_helpers.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/staging/media/meson/vdec/vdec_helpers.c b/drivers/staging/media/meson/vdec/vdec_helpers.c > index f16948b..e7e56d5 100644 > --- a/drivers/staging/media/meson/vdec/vdec_helpers.c > +++ b/drivers/staging/media/meson/vdec/vdec_helpers.c > @@ -206,6 +206,10 @@ void amvdec_add_ts_reorder(struct amvdec_session *sess, u64 ts, u32 offset) > unsigned long flags; > > new_ts = kmalloc(sizeof(*new_ts), GFP_KERNEL); > + if (!new_ts) { > + dev_err(sess->core->dev, "Failed to kmalloc()\n"); Did you run this through checkpatch? I think it will say that this line is not ok. > + return; Shouldn't you return an -ENOMEM error somehow? thanks, greg k-h ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] media: meson: Add NULL check after the call to kmalloc() 2019-09-04 8:45 ` Greg KH @ 2019-09-04 22:39 ` Austin Kim 0 siblings, 0 replies; 5+ messages in thread From: Austin Kim @ 2019-09-04 22:39 UTC (permalink / raw) To: Greg KH Cc: mchehab, khilman, mjourdan, devel, linux-kernel, linux-amlogic, linux-arm-kernel, linux-media 2019년 9월 4일 (수) 오후 5:45, Greg KH <gregkh@linuxfoundation.org>님이 작성: > > On Wed, Sep 04, 2019 at 05:22:32PM +0900, Austin Kim wrote: > > If the kmalloc() return NULL, the NULL pointer dereference will occur. > > new_ts->ts = ts; > > > > Add exception check after the call to kmalloc() is made. > > > > Signed-off-by: Austin Kim <austindh.kim@gmail.com> > > --- > > drivers/staging/media/meson/vdec/vdec_helpers.c | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/drivers/staging/media/meson/vdec/vdec_helpers.c b/drivers/staging/media/meson/vdec/vdec_helpers.c > > index f16948b..e7e56d5 100644 > > --- a/drivers/staging/media/meson/vdec/vdec_helpers.c > > +++ b/drivers/staging/media/meson/vdec/vdec_helpers.c > > @@ -206,6 +206,10 @@ void amvdec_add_ts_reorder(struct amvdec_session *sess, u64 ts, u32 offset) > > unsigned long flags; > > > > new_ts = kmalloc(sizeof(*new_ts), GFP_KERNEL); > > + if (!new_ts) { > > + dev_err(sess->core->dev, "Failed to kmalloc()\n"); > > Did you run this through checkpatch? I think it will say that this line > is not ok. > > > + return; > > Shouldn't you return an -ENOMEM error somehow? I agreed with your feedback. Let me take a look at the code more and then resend the patch if necessary. Thanks, Austin Kim > > thanks, > > greg k-h ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-09-04 22:47 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-09-04 8:22 [PATCH] media: meson: Add NULL check after the call to kmalloc() Austin Kim 2019-09-04 8:43 ` Rasmus Villemoes 2019-09-04 22:47 ` Austin Kim 2019-09-04 8:45 ` Greg KH 2019-09-04 22:39 ` Austin Kim
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).