[PATCH v1] vdpa: Complement vdpa_nl_policy for nlattr length check

[PATCH v1] vdpa: Complement vdpa_nl_policy for nlattr length check

[Lin Ma]

The vdpa_nl_policy structure is used to validate the nlattr when parsing
the incoming nlmsg. It will ensure the attribute being described produces
a valid nlattr pointer in info->attrs before entering into each handler
in vdpa_nl_ops.

That is to say, the missing part in vdpa_nl_policy may lead to illegal
nlattr after parsing, which could lead to OOB read just like CVE-2023-3773.

This patch adds three missing nla_policy to avoid such bugs.

Fixes: 90fea5a800c3 ("vdpa: device feature provisioning")
Fixes: 13b00b135665 ("vdpa: Add support for querying vendor statistics")
Fixes: ad69dd0bf26b ("vdpa: Introduce query of device config layout")
Signed-off-by: Lin Ma <linma@zju.edu.cn>
---
 drivers/vdpa/vdpa.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
index 965e32529eb8..f2f654fd84e5 100644
--- a/drivers/vdpa/vdpa.c
+++ b/drivers/vdpa/vdpa.c
@@ -1247,8 +1247,11 @@ static const struct nla_policy vdpa_nl_policy[VDPA_ATTR_MAX + 1] = {
 	[VDPA_ATTR_MGMTDEV_DEV_NAME] = { .type = NLA_STRING },
 	[VDPA_ATTR_DEV_NAME] = { .type = NLA_STRING },
 	[VDPA_ATTR_DEV_NET_CFG_MACADDR] = NLA_POLICY_ETH_ADDR,
+	[VDPA_ATTR_DEV_NET_CFG_MAX_VQP] = { .type = NLA_U16 },
 	/* virtio spec 1.1 section 5.1.4.1 for valid MTU range */
 	[VDPA_ATTR_DEV_NET_CFG_MTU] = NLA_POLICY_MIN(NLA_U16, 68),
+	[VDPA_ATTR_DEV_QUEUE_INDEX] = { .type = NLA_U32 },
+	[VDPA_ATTR_DEV_FEATURES] = { .type = NLA_U64 },
 };
 
 static const struct genl_ops vdpa_nl_ops[] = {
-- 
2.17.1

Re: [PATCH v1] vdpa: Complement vdpa_nl_policy for nlattr length check

[Michael S. Tsirkin]

On Sun, Jul 23, 2023 at 04:05:07PM +0800, Lin Ma wrote:
> The vdpa_nl_policy structure is used to validate the nlattr when parsing
> the incoming nlmsg. It will ensure the attribute being described produces
> a valid nlattr pointer in info->attrs before entering into each handler
> in vdpa_nl_ops.
> 
> That is to say, the missing part in vdpa_nl_policy may lead to illegal
> nlattr after parsing, which could lead to OOB read just like CVE-2023-3773.

Hmm.

https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2023-3773

** RESERVED ** This candidate has been reserved by an organization or individual that will use it when announcing a new security problem. When the candidate has been publicized, the details for this candidate will be provided.



> This patch adds three missing nla_policy to avoid such bugs.
> 
> Fixes: 90fea5a800c3 ("vdpa: device feature provisioning")
> Fixes: 13b00b135665 ("vdpa: Add support for querying vendor statistics")
> Fixes: ad69dd0bf26b ("vdpa: Introduce query of device config layout")
> Signed-off-by: Lin Ma <linma@zju.edu.cn>

I don't know how OOB triggers but this duplication is problematic I
think: we are likely to forget again in the future.  Isn't there a way
to block everything that is not listed?


> ---
>  drivers/vdpa/vdpa.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> index 965e32529eb8..f2f654fd84e5 100644
> --- a/drivers/vdpa/vdpa.c
> +++ b/drivers/vdpa/vdpa.c
> @@ -1247,8 +1247,11 @@ static const struct nla_policy vdpa_nl_policy[VDPA_ATTR_MAX + 1] = {
>  	[VDPA_ATTR_MGMTDEV_DEV_NAME] = { .type = NLA_STRING },
>  	[VDPA_ATTR_DEV_NAME] = { .type = NLA_STRING },
>  	[VDPA_ATTR_DEV_NET_CFG_MACADDR] = NLA_POLICY_ETH_ADDR,
> +	[VDPA_ATTR_DEV_NET_CFG_MAX_VQP] = { .type = NLA_U16 },
>  	/* virtio spec 1.1 section 5.1.4.1 for valid MTU range */
>  	[VDPA_ATTR_DEV_NET_CFG_MTU] = NLA_POLICY_MIN(NLA_U16, 68),
> +	[VDPA_ATTR_DEV_QUEUE_INDEX] = { .type = NLA_U32 },
> +	[VDPA_ATTR_DEV_FEATURES] = { .type = NLA_U64 },
>  };
>  
>  static const struct genl_ops vdpa_nl_ops[] = {
> -- 
> 2.17.1

Re: [PATCH v1] vdpa: Complement vdpa_nl_policy for nlattr length check

[Lin Ma]

Hello Michael,

> >
> > The vdpa_nl_policy structure is used to validate the nlattr when parsing
> > the incoming nlmsg. It will ensure the attribute being described produces
> > a valid nlattr pointer in info->attrs before entering into each handler
> > in vdpa_nl_ops.
> > 
> > That is to say, the missing part in vdpa_nl_policy may lead to illegal
> > nlattr after parsing, which could lead to OOB read just like CVE-2023-3773.
> 
> Hmm.
> 
> https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2023-3773
> 
> ** RESERVED ** This candidate has been reserved by an organization or individual that will use it when announcing a new security problem. When the candidate has been publicized, the details for this candidate will be provided.
> 

Yeah, that CVE is assigned while fix not upstream yet. FYI, the fix is pending too. 
See, https://marc.info/?l=linux-kernel&m=169009801131058&w=2.


> 
> > This patch adds three missing nla_policy to avoid such bugs.
> > 
> > Fixes: 90fea5a800c3 ("vdpa: device feature provisioning")
> > Fixes: 13b00b135665 ("vdpa: Add support for querying vendor statistics")
> > Fixes: ad69dd0bf26b ("vdpa: Introduce query of device config layout")
> > Signed-off-by: Lin Ma <linma@zju.edu.cn>
> 
> I don't know how OOB triggers but this duplication is problematic I
> think: we are likely to forget again in the future.  Isn't there a way
> to block everything that is not listed?
> 

Sure, that is another undergoing task I'm working on. If the nlattr is parsed with
NL_VALIDATE_UNSPEC, any forgotten nlattr will be rejected, therefore (which is the default
for modern nla_parse). The problem here is that there are still consumers for
nla_parse_deprecated. And we cannot simply replace all *_deprecated to modern ones
as it may break userspace. See the commit message in 8cb081746c03 ("netlink: make
validation more configurable for future strictness")

I believe if we can do enough test against userspace toolchains, we can ultimately
upgrade all *_depprecated parsers to modern ones, which costs time and efforts. This
send patch is a much simpler (but temporary) solution for now.

Regards
Lin

Re: [PATCH v1] vdpa: Complement vdpa_nl_policy for nlattr length check

[Lin Ma]

> Sure, that is another undergoing task I'm working on. If the nlattr is parsed with
> NL_VALIDATE_UNSPEC, any forgotten nlattr will be rejected, therefore (which is the default
> for modern nla_parse). 

For the general netlink interface, the deciding flag should be genl_ops.validate defined in 
each ops. The default validate flag is strict, while the developer can overwrite the flag 
with GENL_DONT_VALIDATE_STRICT to ease the validation. That is to say, safer code should 
enforce NL_VALIDATE_STRICT by not overwriting the validate flag.

Regrads
Lin

Re: [PATCH v1] vdpa: Complement vdpa_nl_policy for nlattr length check

[Michael S. Tsirkin]

On Sun, Jul 23, 2023 at 05:48:46PM +0800, Lin Ma wrote:
> 
> > Sure, that is another undergoing task I'm working on. If the nlattr is parsed with
> > NL_VALIDATE_UNSPEC, any forgotten nlattr will be rejected, therefore (which is the default
> > for modern nla_parse). 
> 
> For the general netlink interface, the deciding flag should be genl_ops.validate defined in 
> each ops. The default validate flag is strict, while the developer can overwrite the flag 
> with GENL_DONT_VALIDATE_STRICT to ease the validation. That is to say, safer code should 
> enforce NL_VALIDATE_STRICT by not overwriting the validate flag.
> 
> Regrads
> Lin


Oh I see.

It started here:

commit 33b347503f014ebf76257327cbc7001c6b721956
Author: Parav Pandit <parav@nvidia.com>
Date:   Tue Jan 5 12:32:00 2021 +0200

    vdpa: Define vdpa mgmt device, ops and a netlink interface

which did:

+               .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,


which was most likely just a copy paste from somewhere, right Parav?

and then everyone kept copying this around.

Parav, Eli can we drop these? There's a tiny chance of breaking something
but I feel there aren't that many users outside mlx5 yet, so if you
guys can test on mlx5 and confirm no breakage, I think we are good.

-- 
MST

Re: [PATCH v1] vdpa: Complement vdpa_nl_policy for nlattr length check

[Jason Wang]

On Sun, Jul 23, 2023 at 6:02 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Sun, Jul 23, 2023 at 05:48:46PM +0800, Lin Ma wrote:
> >
> > > Sure, that is another undergoing task I'm working on. If the nlattr is parsed with
> > > NL_VALIDATE_UNSPEC, any forgotten nlattr will be rejected, therefore (which is the default
> > > for modern nla_parse).
> >
> > For the general netlink interface, the deciding flag should be genl_ops.validate defined in
> > each ops. The default validate flag is strict, while the developer can overwrite the flag
> > with GENL_DONT_VALIDATE_STRICT to ease the validation. That is to say, safer code should
> > enforce NL_VALIDATE_STRICT by not overwriting the validate flag.
> >
> > Regrads
> > Lin
>
>
> Oh I see.
>
> It started here:
>
> commit 33b347503f014ebf76257327cbc7001c6b721956
> Author: Parav Pandit <parav@nvidia.com>
> Date:   Tue Jan 5 12:32:00 2021 +0200
>
>     vdpa: Define vdpa mgmt device, ops and a netlink interface
>
> which did:
>
> +               .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
>
>
> which was most likely just a copy paste from somewhere, right Parav?
>
> and then everyone kept copying this around.
>
> Parav, Eli can we drop these? There's a tiny chance of breaking something
> but I feel there aren't that many users outside mlx5 yet, so if you
> guys can test on mlx5 and confirm no breakage, I think we are good.

Adding Dragos.

Thanks

>
> --
> MST
>

Re: [PATCH v1] vdpa: Complement vdpa_nl_policy for nlattr length check

[Dragos Tatulea]

On Mon, 2023-07-24 at 15:11 +0800, Jason Wang wrote:
> On Sun, Jul 23, 2023 at 6:02 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > 
> > On Sun, Jul 23, 2023 at 05:48:46PM +0800, Lin Ma wrote:
> > > 
> > > > Sure, that is another undergoing task I'm working on. If the nlattr is
> > > > parsed with
> > > > NL_VALIDATE_UNSPEC, any forgotten nlattr will be rejected, therefore
> > > > (which is the default
> > > > for modern nla_parse).
> > > 
> > > For the general netlink interface, the deciding flag should be
> > > genl_ops.validate defined in
> > > each ops. The default validate flag is strict, while the developer can
> > > overwrite the flag
> > > with GENL_DONT_VALIDATE_STRICT to ease the validation. That is to say,
> > > safer code should
> > > enforce NL_VALIDATE_STRICT by not overwriting the validate flag.
> > > 
> > > Regrads
> > > Lin
> > 
> > 
> > Oh I see.
> > 
> > It started here:
> > 
> > commit 33b347503f014ebf76257327cbc7001c6b721956
> > Author: Parav Pandit <parav@nvidia.com>
> > Date:   Tue Jan 5 12:32:00 2021 +0200
> > 
> >     vdpa: Define vdpa mgmt device, ops and a netlink interface
> > 
> > which did:
> > 
> > +               .validate = GENL_DONT_VALIDATE_STRICT |
> > GENL_DONT_VALIDATE_DUMP,
> > 
> > 
> > which was most likely just a copy paste from somewhere, right Parav?
> > 
> > and then everyone kept copying this around.
> > 
> > Parav, Eli can we drop these? There's a tiny chance of breaking something
> > but I feel there aren't that many users outside mlx5 yet, so if you
> > guys can test on mlx5 and confirm no breakage, I think we are good.
> 
> Adding Dragos.
> 
I will check. Just to make sure I understand correctly: you want me to drop the
.validate flags all together in all vdpa ops and check, right?

Thanks,
Dragos

Re: [PATCH v1] vdpa: Complement vdpa_nl_policy for nlattr length check

[Michael S. Tsirkin]

On Mon, Jul 24, 2023 at 08:38:04AM +0000, Dragos Tatulea wrote:
> 
> On Mon, 2023-07-24 at 15:11 +0800, Jason Wang wrote:
> > On Sun, Jul 23, 2023 at 6:02 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > 
> > > On Sun, Jul 23, 2023 at 05:48:46PM +0800, Lin Ma wrote:
> > > > 
> > > > > Sure, that is another undergoing task I'm working on. If the nlattr is
> > > > > parsed with
> > > > > NL_VALIDATE_UNSPEC, any forgotten nlattr will be rejected, therefore
> > > > > (which is the default
> > > > > for modern nla_parse).
> > > > 
> > > > For the general netlink interface, the deciding flag should be
> > > > genl_ops.validate defined in
> > > > each ops. The default validate flag is strict, while the developer can
> > > > overwrite the flag
> > > > with GENL_DONT_VALIDATE_STRICT to ease the validation. That is to say,
> > > > safer code should
> > > > enforce NL_VALIDATE_STRICT by not overwriting the validate flag.
> > > > 
> > > > Regrads
> > > > Lin
> > > 
> > > 
> > > Oh I see.
> > > 
> > > It started here:
> > > 
> > > commit 33b347503f014ebf76257327cbc7001c6b721956
> > > Author: Parav Pandit <parav@nvidia.com>
> > > Date:   Tue Jan 5 12:32:00 2021 +0200
> > > 
> > >     vdpa: Define vdpa mgmt device, ops and a netlink interface
> > > 
> > > which did:
> > > 
> > > +               .validate = GENL_DONT_VALIDATE_STRICT |
> > > GENL_DONT_VALIDATE_DUMP,
> > > 
> > > 
> > > which was most likely just a copy paste from somewhere, right Parav?
> > > 
> > > and then everyone kept copying this around.
> > > 
> > > Parav, Eli can we drop these? There's a tiny chance of breaking something
> > > but I feel there aren't that many users outside mlx5 yet, so if you
> > > guys can test on mlx5 and confirm no breakage, I think we are good.
> > 
> > Adding Dragos.
> > 
> I will check. Just to make sure I understand correctly: you want me to drop the
> .validate flags all together in all vdpa ops and check, right?
> 
> Thanks,
> Dragos

yes - I suspect you will then need this patch to make things work.

-- 
MST

Re: [PATCH v1] vdpa: Complement vdpa_nl_policy for nlattr length check

[Dragos Tatulea]

On Mon, 2023-07-24 at 05:16 -0400, Michael S. Tsirkin wrote:
> On Mon, Jul 24, 2023 at 08:38:04AM +0000, Dragos Tatulea wrote:
> > 
> > On Mon, 2023-07-24 at 15:11 +0800, Jason Wang wrote:
> > > On Sun, Jul 23, 2023 at 6:02 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > 
> > > > On Sun, Jul 23, 2023 at 05:48:46PM +0800, Lin Ma wrote:
> > > > > 
> > > > > > Sure, that is another undergoing task I'm working on. If the nlattr
> > > > > > is
> > > > > > parsed with
> > > > > > NL_VALIDATE_UNSPEC, any forgotten nlattr will be rejected, therefore
> > > > > > (which is the default
> > > > > > for modern nla_parse).
> > > > > 
> > > > > For the general netlink interface, the deciding flag should be
> > > > > genl_ops.validate defined in
> > > > > each ops. The default validate flag is strict, while the developer can
> > > > > overwrite the flag
> > > > > with GENL_DONT_VALIDATE_STRICT to ease the validation. That is to say,
> > > > > safer code should
> > > > > enforce NL_VALIDATE_STRICT by not overwriting the validate flag.
> > > > > 
> > > > > Regrads
> > > > > Lin
> > > > 
> > > > 
> > > > Oh I see.
> > > > 
> > > > It started here:
> > > > 
> > > > commit 33b347503f014ebf76257327cbc7001c6b721956
> > > > Author: Parav Pandit <parav@nvidia.com>
> > > > Date:   Tue Jan 5 12:32:00 2021 +0200
> > > > 
> > > >     vdpa: Define vdpa mgmt device, ops and a netlink interface
> > > > 
> > > > which did:
> > > > 
> > > > +               .validate = GENL_DONT_VALIDATE_STRICT |
> > > > GENL_DONT_VALIDATE_DUMP,
> > > > 
> > > > 
> > > > which was most likely just a copy paste from somewhere, right Parav?
> > > > 
> > > > and then everyone kept copying this around.
> > > > 
> > > > Parav, Eli can we drop these? There's a tiny chance of breaking
> > > > something
> > > > but I feel there aren't that many users outside mlx5 yet, so if you
> > > > guys can test on mlx5 and confirm no breakage, I think we are good.
> > > 
> > > Adding Dragos.
> > > 
> > I will check. Just to make sure I understand correctly: you want me to drop
> > the
> > .validate flags all together in all vdpa ops and check, right?
> > 
> > Thanks,
> > Dragos
> 
> yes - I suspect you will then need this patch to make things work.
> 
Yep. Adding the patch and removing the .validate config on the vdpa_nl_ops
seems to work just fine.

Thanks,
Dragos

Re: [PATCH v1] vdpa: Complement vdpa_nl_policy for nlattr length check

[Michael S. Tsirkin]

On Mon, Jul 24, 2023 at 11:42:42AM +0000, Dragos Tatulea wrote:
> On Mon, 2023-07-24 at 05:16 -0400, Michael S. Tsirkin wrote:
> > On Mon, Jul 24, 2023 at 08:38:04AM +0000, Dragos Tatulea wrote:
> > > 
> > > On Mon, 2023-07-24 at 15:11 +0800, Jason Wang wrote:
> > > > On Sun, Jul 23, 2023 at 6:02 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > > 
> > > > > On Sun, Jul 23, 2023 at 05:48:46PM +0800, Lin Ma wrote:
> > > > > > 
> > > > > > > Sure, that is another undergoing task I'm working on. If the nlattr
> > > > > > > is
> > > > > > > parsed with
> > > > > > > NL_VALIDATE_UNSPEC, any forgotten nlattr will be rejected, therefore
> > > > > > > (which is the default
> > > > > > > for modern nla_parse).
> > > > > > 
> > > > > > For the general netlink interface, the deciding flag should be
> > > > > > genl_ops.validate defined in
> > > > > > each ops. The default validate flag is strict, while the developer can
> > > > > > overwrite the flag
> > > > > > with GENL_DONT_VALIDATE_STRICT to ease the validation. That is to say,
> > > > > > safer code should
> > > > > > enforce NL_VALIDATE_STRICT by not overwriting the validate flag.
> > > > > > 
> > > > > > Regrads
> > > > > > Lin
> > > > > 
> > > > > 
> > > > > Oh I see.
> > > > > 
> > > > > It started here:
> > > > > 
> > > > > commit 33b347503f014ebf76257327cbc7001c6b721956
> > > > > Author: Parav Pandit <parav@nvidia.com>
> > > > > Date:   Tue Jan 5 12:32:00 2021 +0200
> > > > > 
> > > > >     vdpa: Define vdpa mgmt device, ops and a netlink interface
> > > > > 
> > > > > which did:
> > > > > 
> > > > > +               .validate = GENL_DONT_VALIDATE_STRICT |
> > > > > GENL_DONT_VALIDATE_DUMP,
> > > > > 
> > > > > 
> > > > > which was most likely just a copy paste from somewhere, right Parav?
> > > > > 
> > > > > and then everyone kept copying this around.
> > > > > 
> > > > > Parav, Eli can we drop these? There's a tiny chance of breaking
> > > > > something
> > > > > but I feel there aren't that many users outside mlx5 yet, so if you
> > > > > guys can test on mlx5 and confirm no breakage, I think we are good.
> > > > 
> > > > Adding Dragos.
> > > > 
> > > I will check. Just to make sure I understand correctly: you want me to drop
> > > the
> > > .validate flags all together in all vdpa ops and check, right?
> > > 
> > > Thanks,
> > > Dragos
> > 
> > yes - I suspect you will then need this patch to make things work.
> > 
> Yep. Adding the patch and removing the .validate config on the vdpa_nl_ops
> seems to work just fine.
> 
> Thanks,
> Dragos

OK, post a patch?

-- 
MST

Re: [PATCH v1] vdpa: Complement vdpa_nl_policy for nlattr length check

[Dragos Tatulea]

On Mon, 2023-07-24 at 16:08 -0400, Michael S. Tsirkin wrote:
> On Mon, Jul 24, 2023 at 11:42:42AM +0000, Dragos Tatulea wrote:
> > On Mon, 2023-07-24 at 05:16 -0400, Michael S. Tsirkin wrote:
> > > On Mon, Jul 24, 2023 at 08:38:04AM +0000, Dragos Tatulea wrote:
> > > > 
> > > > On Mon, 2023-07-24 at 15:11 +0800, Jason Wang wrote:
> > > > > On Sun, Jul 23, 2023 at 6:02 PM Michael S. Tsirkin <mst@redhat.com>
> > > > > wrote:
> > > > > > 
> > > > > > On Sun, Jul 23, 2023 at 05:48:46PM +0800, Lin Ma wrote:
> > > > > > > 
> > > > > > > > Sure, that is another undergoing task I'm working on. If the
> > > > > > > > nlattr
> > > > > > > > is
> > > > > > > > parsed with
> > > > > > > > NL_VALIDATE_UNSPEC, any forgotten nlattr will be rejected,
> > > > > > > > therefore
> > > > > > > > (which is the default
> > > > > > > > for modern nla_parse).
> > > > > > > 
> > > > > > > For the general netlink interface, the deciding flag should be
> > > > > > > genl_ops.validate defined in
> > > > > > > each ops. The default validate flag is strict, while the developer
> > > > > > > can
> > > > > > > overwrite the flag
> > > > > > > with GENL_DONT_VALIDATE_STRICT to ease the validation. That is to
> > > > > > > say,
> > > > > > > safer code should
> > > > > > > enforce NL_VALIDATE_STRICT by not overwriting the validate flag.
> > > > > > > 
> > > > > > > Regrads
> > > > > > > Lin
> > > > > > 
> > > > > > 
> > > > > > Oh I see.
> > > > > > 
> > > > > > It started here:
> > > > > > 
> > > > > > commit 33b347503f014ebf76257327cbc7001c6b721956
> > > > > > Author: Parav Pandit <parav@nvidia.com>
> > > > > > Date:   Tue Jan 5 12:32:00 2021 +0200
> > > > > > 
> > > > > >     vdpa: Define vdpa mgmt device, ops and a netlink interface
> > > > > > 
> > > > > > which did:
> > > > > > 
> > > > > > +               .validate = GENL_DONT_VALIDATE_STRICT |
> > > > > > GENL_DONT_VALIDATE_DUMP,
> > > > > > 
> > > > > > 
> > > > > > which was most likely just a copy paste from somewhere, right Parav?
> > > > > > 
> > > > > > and then everyone kept copying this around.
> > > > > > 
> > > > > > Parav, Eli can we drop these? There's a tiny chance of breaking
> > > > > > something
> > > > > > but I feel there aren't that many users outside mlx5 yet, so if you
> > > > > > guys can test on mlx5 and confirm no breakage, I think we are good.
> > > > > 
> > > > > Adding Dragos.
> > > > > 
> > > > I will check. Just to make sure I understand correctly: you want me to
> > > > drop
> > > > the
> > > > .validate flags all together in all vdpa ops and check, right?
> > > > 
> > > > Thanks,
> > > > Dragos
> > > 
> > > yes - I suspect you will then need this patch to make things work.
> > > 
> > Yep. Adding the patch and removing the .validate config on the vdpa_nl_ops
> > seems to work just fine.
> > 
> > Thanks,
> > Dragos
> 
> OK, post a patch?
> 
Sure, but how do I make it depend on this patch? Otherwise it will break things.

Thanks,
Dragos

Re: [PATCH v1] vdpa: Complement vdpa_nl_policy for nlattr length check

[Michael S. Tsirkin]

On Tue, Jul 25, 2023 at 08:26:32AM +0000, Dragos Tatulea wrote:
> On Mon, 2023-07-24 at 16:08 -0400, Michael S. Tsirkin wrote:
> > On Mon, Jul 24, 2023 at 11:42:42AM +0000, Dragos Tatulea wrote:
> > > On Mon, 2023-07-24 at 05:16 -0400, Michael S. Tsirkin wrote:
> > > > On Mon, Jul 24, 2023 at 08:38:04AM +0000, Dragos Tatulea wrote:
> > > > > 
> > > > > On Mon, 2023-07-24 at 15:11 +0800, Jason Wang wrote:
> > > > > > On Sun, Jul 23, 2023 at 6:02 PM Michael S. Tsirkin <mst@redhat.com>
> > > > > > wrote:
> > > > > > > 
> > > > > > > On Sun, Jul 23, 2023 at 05:48:46PM +0800, Lin Ma wrote:
> > > > > > > > 
> > > > > > > > > Sure, that is another undergoing task I'm working on. If the
> > > > > > > > > nlattr
> > > > > > > > > is
> > > > > > > > > parsed with
> > > > > > > > > NL_VALIDATE_UNSPEC, any forgotten nlattr will be rejected,
> > > > > > > > > therefore
> > > > > > > > > (which is the default
> > > > > > > > > for modern nla_parse).
> > > > > > > > 
> > > > > > > > For the general netlink interface, the deciding flag should be
> > > > > > > > genl_ops.validate defined in
> > > > > > > > each ops. The default validate flag is strict, while the developer
> > > > > > > > can
> > > > > > > > overwrite the flag
> > > > > > > > with GENL_DONT_VALIDATE_STRICT to ease the validation. That is to
> > > > > > > > say,
> > > > > > > > safer code should
> > > > > > > > enforce NL_VALIDATE_STRICT by not overwriting the validate flag.
> > > > > > > > 
> > > > > > > > Regrads
> > > > > > > > Lin
> > > > > > > 
> > > > > > > 
> > > > > > > Oh I see.
> > > > > > > 
> > > > > > > It started here:
> > > > > > > 
> > > > > > > commit 33b347503f014ebf76257327cbc7001c6b721956
> > > > > > > Author: Parav Pandit <parav@nvidia.com>
> > > > > > > Date:   Tue Jan 5 12:32:00 2021 +0200
> > > > > > > 
> > > > > > >     vdpa: Define vdpa mgmt device, ops and a netlink interface
> > > > > > > 
> > > > > > > which did:
> > > > > > > 
> > > > > > > +               .validate = GENL_DONT_VALIDATE_STRICT |
> > > > > > > GENL_DONT_VALIDATE_DUMP,
> > > > > > > 
> > > > > > > 
> > > > > > > which was most likely just a copy paste from somewhere, right Parav?
> > > > > > > 
> > > > > > > and then everyone kept copying this around.
> > > > > > > 
> > > > > > > Parav, Eli can we drop these? There's a tiny chance of breaking
> > > > > > > something
> > > > > > > but I feel there aren't that many users outside mlx5 yet, so if you
> > > > > > > guys can test on mlx5 and confirm no breakage, I think we are good.
> > > > > > 
> > > > > > Adding Dragos.
> > > > > > 
> > > > > I will check. Just to make sure I understand correctly: you want me to
> > > > > drop
> > > > > the
> > > > > .validate flags all together in all vdpa ops and check, right?
> > > > > 
> > > > > Thanks,
> > > > > Dragos
> > > > 
> > > > yes - I suspect you will then need this patch to make things work.
> > > > 
> > > Yep. Adding the patch and removing the .validate config on the vdpa_nl_ops
> > > seems to work just fine.
> > > 
> > > Thanks,
> > > Dragos
> > 
> > OK, post a patch?
> > 
> Sure, but how do I make it depend on this patch? Otherwise it will break things.
> 
> Thanks,
> Dragos

Send a patch series with this as patch 1/2 that one 2/2.

Re: [PATCH v1] vdpa: Complement vdpa_nl_policy for nlattr length check

[Michael S. Tsirkin]

On Sun, Jul 23, 2023 at 05:33:54PM +0800, Lin Ma wrote:
> Hello Michael,
> 
> > >
> > > The vdpa_nl_policy structure is used to validate the nlattr when parsing
> > > the incoming nlmsg. It will ensure the attribute being described produces
> > > a valid nlattr pointer in info->attrs before entering into each handler
> > > in vdpa_nl_ops.
> > > 
> > > That is to say, the missing part in vdpa_nl_policy may lead to illegal
> > > nlattr after parsing, which could lead to OOB read just like CVE-2023-3773.
> > 
> > Hmm.
> > 
> > https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2023-3773
> > 
> > ** RESERVED ** This candidate has been reserved by an organization or individual that will use it when announcing a new security problem. When the candidate has been publicized, the details for this candidate will be provided.
> > 
> 
> Yeah, that CVE is assigned while fix not upstream yet. FYI, the fix is pending too. 
> See, https://marc.info/?l=linux-kernel&m=169009801131058&w=2.
> 
> > 
> > > This patch adds three missing nla_policy to avoid such bugs.
> > > 
> > > Fixes: 90fea5a800c3 ("vdpa: device feature provisioning")
> > > Fixes: 13b00b135665 ("vdpa: Add support for querying vendor statistics")
> > > Fixes: ad69dd0bf26b ("vdpa: Introduce query of device config layout")
> > > Signed-off-by: Lin Ma <linma@zju.edu.cn>
> > 
> > I don't know how OOB triggers but this duplication is problematic I
> > think: we are likely to forget again in the future.  Isn't there a way
> > to block everything that is not listed?
> > 
> 
> Sure, that is another undergoing task I'm working on. If the nlattr is parsed with
> NL_VALIDATE_UNSPEC, any forgotten nlattr will be rejected, therefore (which is the default
> for modern nla_parse). The problem here is that there are still consumers for
> nla_parse_deprecated. And we cannot simply replace all *_deprecated to modern ones
> as it may break userspace. See the commit message in 8cb081746c03 ("netlink: make
> validation more configurable for future strictness")
> 
> I believe if we can do enough test against userspace toolchains, we can ultimately
> upgrade all *_depprecated parsers to modern ones, which costs time and efforts. This
> send patch is a much simpler (but temporary) solution for now.
> 
> Regards
> Lin

Hmm but vdpa does not use nla_parse_deprecated does it? And in fact was
introduced after 8cb081746c031fb164089322e2336a0bf5b3070c.
So why is there an issue in vdpa?

-- 
MST