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

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

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

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

> 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

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