2019-07-24 02:34:51

by Navid Emamdoost

Subject: [PATCH] nbd_genl_status: null check for nla_nest_start

nla_nest_start may fail and return NULL. The check is inserted, and
errno is selected based on other call sites within the same source code.

Signed-off-by: Navid Emamdoost <[email protected]>
---
drivers/block/nbd.c | 6 ++++++
1 file changed, 6 insertions(+)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 9bcde2325893..dba362de4d91 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -2149,6 +2149,12 @@ static int nbd_genl_status(struct sk_buff *skb, struct genl_info *info)
}

dev_list = nla_nest_start_noflag(reply, NBD_ATTR_DEVICE_LIST);
+
+ if (!dev_list) {
+ ret = -EMSGSIZE;
+ goto out;
+ }
+
if (index == -1) {
ret = idr_for_each(&nbd_index_idr, &status_cb, reply);
if (ret) {
--
2.17.1

2019-07-29 14:01:50

by Josef Bacik

Subject: Re: [PATCH] nbd_genl_status: null check for nla_nest_start

On Tue, Jul 23, 2019 at 06:01:57PM -0500, Navid Emamdoost wrote:
> nla_nest_start may fail and return NULL. The check is inserted, and
> errno is selected based on other call sites within the same source code.
>
> Signed-off-by: Navid Emamdoost <[email protected]>
> ---
> drivers/block/nbd.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> index 9bcde2325893..dba362de4d91 100644
> --- a/drivers/block/nbd.c
> +++ b/drivers/block/nbd.c
> @@ -2149,6 +2149,12 @@ static int nbd_genl_status(struct sk_buff *skb, struct genl_info *info)
> }
>
> dev_list = nla_nest_start_noflag(reply, NBD_ATTR_DEVICE_LIST);
> +

No newline here, once you fix that nit you can add

Reviewed-by: Josef Bacik <[email protected]>

Thanks,

Josef

2019-07-29 21:58:23

by Navid Emamdoost

Subject: [PATCH v2] nbd_genl_status: null check for nla_nest_start

nla_nest_start may fail and return NULL. The check is inserted, and
errno is selected based on other call sites within the same source code.
Update: removed extra new line.

Signed-off-by: Navid Emamdoost <[email protected]>
---
drivers/block/nbd.c | 5 +++++
1 file changed, 5 insertions(+)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 9bcde2325893..2410812d1e82 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -2149,6 +2149,11 @@ static int nbd_genl_status(struct sk_buff *skb, struct genl_info *info)
}

dev_list = nla_nest_start_noflag(reply, NBD_ATTR_DEVICE_LIST);
+ if (!dev_list) {
+ ret = -EMSGSIZE;
+ goto out;
+ }
+
if (index == -1) {
ret = idr_for_each(&nbd_index_idr, &status_cb, reply);
if (ret) {
--
2.17.1

2019-07-30 05:58:11

by Red Hat Product Security

Subject: [engineering.redhat.com #494735] Re: [PATCH] nbd_genl_status: null check for nla_nest_start

Hi Navid,

Thank you for you report. I have forwarded this to our analysis team. Once I'll
get an update on your reported vulnerability and it's patched I'll let you
know.
Please let me know if you have any questions or concerns.

On Mon Jul 29 12:42:56 2019, [email protected] wrote:
> nla_nest_start may fail and return NULL. The check is inserted, and
> errno is selected based on other call sites within the same source
> code.
> Update: removed extra new line.
>
> Signed-off-by: Navid Emamdoost <[email protected]>
> ---
> drivers/block/nbd.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> index 9bcde2325893..2410812d1e82 100644
> --- a/drivers/block/nbd.c
> +++ b/drivers/block/nbd.c
> @@ -2149,6 +2149,11 @@ static int nbd_genl_status(struct sk_buff *skb,
> struct genl_info *info)
> }
>
> dev_list = nla_nest_start_noflag(reply, NBD_ATTR_DEVICE_LIST);
> + if (!dev_list) {
> + ret = -EMSGSIZE;
> + goto out;
> + }
> +
> if (index == -1) {
> ret = idr_for_each(&nbd_index_idr, &status_cb, reply);
> if (ret) {


--
Best Regards,
Dhananjay Arunesh, Red Hat Product Security
7F45 FDD1 BB92 2DA8 CD05 F034 9B3D 8FE3 50EC 5D74

2019-07-30 06:09:07

by Bob Liu

Subject: Re: [PATCH v2] nbd_genl_status: null check for nla_nest_start

On 7/30/19 12:42 AM, Navid Emamdoost wrote:
> nla_nest_start may fail and return NULL. The check is inserted, and
> errno is selected based on other call sites within the same source code.
> Update: removed extra new line.
>
> Signed-off-by: Navid Emamdoost <[email protected]>
> ---
> drivers/block/nbd.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> index 9bcde2325893..2410812d1e82 100644
> --- a/drivers/block/nbd.c
> +++ b/drivers/block/nbd.c
> @@ -2149,6 +2149,11 @@ static int nbd_genl_status(struct sk_buff *skb, struct genl_info *info)
> }
>
> dev_list = nla_nest_start_noflag(reply, NBD_ATTR_DEVICE_LIST);
> + if (!dev_list) {
> + ret = -EMSGSIZE;
> + goto out;
> + }
> +
> if (index == -1) {
> ret = idr_for_each(&nbd_index_idr, &status_cb, reply);
> if (ret) {
>

Looks good to me.
Reviewed-by: Bob Liu <[email protected]>

2019-09-10 18:58:06

by Michal Kubecek

Subject: Re: [PATCH v2] nbd_genl_status: null check for nla_nest_start

(Just stumbled upon this patch when link to it came with a CVE bug report.)

On Mon, Jul 29, 2019 at 11:42:26AM -0500, Navid Emamdoost wrote:
> nla_nest_start may fail and return NULL. The check is inserted, and
> errno is selected based on other call sites within the same source code.
> Update: removed extra new line.
>
> Signed-off-by: Navid Emamdoost <[email protected]>
> Reviewed-by: Bob Liu <[email protected]>
> ---
> drivers/block/nbd.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> index 9bcde2325893..2410812d1e82 100644
> --- a/drivers/block/nbd.c
> +++ b/drivers/block/nbd.c
> @@ -2149,6 +2149,11 @@ static int nbd_genl_status(struct sk_buff *skb, struct genl_info *info)
> }
>
> dev_list = nla_nest_start_noflag(reply, NBD_ATTR_DEVICE_LIST);
> + if (!dev_list) {
> + ret = -EMSGSIZE;
> + goto out;
> + }
> +
> if (index == -1) {
> ret = idr_for_each(&nbd_index_idr, &status_cb, reply);
> if (ret) {

You should also call nlmsg_free(reply) when you bail out so that you
don't introduce a memory leak.

Michal Kubecek

2019-09-11 16:42:43

by Navid Emamdoost

Subject: [PATCH v3] nbd_genl_status: null check for nla_nest_start

nla_nest_start may fail and return NULL. The check is inserted, and
errno is selected based on other call sites within the same source code.
Update: removed extra new line.
v3 Update: added release reply, thanks to Michal Kubecek for pointing
out.

Signed-off-by: Navid Emamdoost <[email protected]>
---
drivers/block/nbd.c | 6 ++++++
1 file changed, 6 insertions(+)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index e21d2ded732b..8a9712181c2a 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -2149,6 +2149,12 @@ static int nbd_genl_status(struct sk_buff *skb, struct genl_info *info)
}

dev_list = nla_nest_start_noflag(reply, NBD_ATTR_DEVICE_LIST);
+ if (!dev_list) {
+ nlmsg_free(reply);
+ ret = -EMSGSIZE;
+ goto out;
+ }
+
if (index == -1) {
ret = idr_for_each(&nbd_index_idr, &status_cb, reply);
if (ret) {
--
2.17.1

2019-10-18 05:44:02

by Navid Emamdoost

Subject: Re: [PATCH v2] nbd_genl_status: null check for nla_nest_start

Hi Michal, please check v3 at https://lore.kernel.org/patchwork/patch/1126650/


Thanks,
Navid.

On Tue, Sep 10, 2019 at 6:35 AM Michal Kubecek <[email protected]> wrote:
>
> (Just stumbled upon this patch when link to it came with a CVE bug report.)
>
> On Mon, Jul 29, 2019 at 11:42:26AM -0500, Navid Emamdoost wrote:
> > nla_nest_start may fail and return NULL. The check is inserted, and
> > errno is selected based on other call sites within the same source code.
> > Update: removed extra new line.
> >
> > Signed-off-by: Navid Emamdoost <[email protected]>
> > Reviewed-by: Bob Liu <[email protected]>
> > ---
> > drivers/block/nbd.c | 5 +++++
> > 1 file changed, 5 insertions(+)
> >
> > diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> > index 9bcde2325893..2410812d1e82 100644
> > --- a/drivers/block/nbd.c
> > +++ b/drivers/block/nbd.c
> > @@ -2149,6 +2149,11 @@ static int nbd_genl_status(struct sk_buff *skb, struct genl_info *info)
> > }
> >
> > dev_list = nla_nest_start_noflag(reply, NBD_ATTR_DEVICE_LIST);
> > + if (!dev_list) {
> > + ret = -EMSGSIZE;
> > + goto out;
> > + }
> > +
> > if (index == -1) {
> > ret = idr_for_each(&nbd_index_idr, &status_cb, reply);
> > if (ret) {
>
> You should also call nlmsg_free(reply) when you bail out so that you
> don't introduce a memory leak.
>
> Michal Kubecek



--
Navid.

2019-10-18 22:16:55

by Red Hat Product Security

Subject: [engineering.redhat.com #498403] Re: [PATCH v2] nbd_genl_status: null check for nla_nest_start

Hi Navid,

Not sure if you meant to cc [email protected] on this. If anything is needed
from our side please let us know!

On Wed Oct 16 22:17:42 2019, [email protected] wrote:
> Hi Michal, please check v3 at
> https://lore.kernel.org/patchwork/patch/1126650/
>
>
> Thanks,
> Navid.
>
> On Tue, Sep 10, 2019 at 6:35 AM Michal Kubecek <[email protected]>
> wrote:
> >
> > (Just stumbled upon this patch when link to it came with a CVE bug
> report.)
> >
> > On Mon, Jul 29, 2019 at 11:42:26AM -0500, Navid Emamdoost wrote:
> > > nla_nest_start may fail and return NULL. The check is inserted,
> and
> > > errno is selected based on other call sites within the same source
> code.
> > > Update: removed extra new line.
> > >
> > > Signed-off-by: Navid Emamdoost <[email protected]>
> > > Reviewed-by: Bob Liu <[email protected]>
> > > ---
> > > drivers/block/nbd.c | 5 +++++
> > > 1 file changed, 5 insertions(+)
> > >
> > > diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> > > index 9bcde2325893..2410812d1e82 100644
> > > --- a/drivers/block/nbd.c
> > > +++ b/drivers/block/nbd.c
> > > @@ -2149,6 +2149,11 @@ static int nbd_genl_status(struct sk_buff
> *skb, struct genl_info *info)
> > > }
> > >
> > > dev_list = nla_nest_start_noflag(reply,
> NBD_ATTR_DEVICE_LIST);
> > > + if (!dev_list) {
> > > + ret = -EMSGSIZE;
> > > + goto out;
> > > + }
> > > +
> > > if (index == -1) {
> > > ret = idr_for_each(&nbd_index_idr, &status_cb,
> reply);
> > > if (ret) {
> >
> > You should also call nlmsg_free(reply) when you bail out so that you
> > don't introduce a memory leak.
> >
> > Michal Kubecek
>
>
>


--
Kat Bost
Red Hat Product Security

2019-10-21 06:43:06

by Michal Kubecek

Subject: Re: [PATCH v3] nbd_genl_status: null check for nla_nest_start

On Wed, Sep 11, 2019 at 11:40:12AM -0500, Navid Emamdoost wrote:
> nla_nest_start may fail and return NULL. The check is inserted, and
> errno is selected based on other call sites within the same source code.
> Update: removed extra new line.
> v3 Update: added release reply, thanks to Michal Kubecek for pointing
> out.
>
> Signed-off-by: Navid Emamdoost <[email protected]>
> ---
> drivers/block/nbd.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> index e21d2ded732b..8a9712181c2a 100644
> --- a/drivers/block/nbd.c
> +++ b/drivers/block/nbd.c
> @@ -2149,6 +2149,12 @@ static int nbd_genl_status(struct sk_buff *skb, struct genl_info *info)
> }
>
> dev_list = nla_nest_start_noflag(reply, NBD_ATTR_DEVICE_LIST);
> + if (!dev_list) {
> + nlmsg_free(reply);
> + ret = -EMSGSIZE;
> + goto out;
> + }
> +
> if (index == -1) {
> ret = idr_for_each(&nbd_index_idr, &status_cb, reply);
> if (ret) {

Reviewed-by: Michal Kubecek <[email protected]>

2021-04-14 12:19:04

by Mark-PK Tsai (蔡沛剛)

Subject: Re: [PATCH v3] nbd_genl_status: null check for nla_nest_start

From: Michal Kubecek <[email protected]>

Hi,

I found that CVE-2019-16089 still exist in upstream kernel.
Does anyone know why this patch was not merged?