| Message ID | 20190904082232.GA171180@LGEARND20B15 (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | media: meson: Add NULL check after the call to kmalloc() | expand |
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
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
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
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
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;
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(+)