| Message ID | 20230725023330.422856-1-linma@zju.edu.cn (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | BPF |
| Headers | show |
| Series | [v2] bpf: Add length check for SK_DIAG_BPF_STORAGE_REQ_MAP_FD parsing | expand |
On Tue, Jul 25, 2023 at 10:33:30AM +0800, Lin Ma wrote: > The nla_for_each_nested parsing in function bpf_sk_storage_diag_alloc > does not check the length of the nested attribute. This can lead to an > out-of-attribute read and allow a malformed nlattr (e.g., length 0) to > be viewed as a 4 byte integer. > > This patch adds an additional check when the nlattr is getting counted. > This makes sure the latter nla_get_u32 can access the attributes with > the correct length. > > Fixes: 1ed4d92458a9 ("bpf: INET_DIAG support in bpf_sk_storage") > Suggested-by: Jakub Kicinski <kuba@kernel.org> > Signed-off-by: Lin Ma <linma@zju.edu.cn> > --- > V1 -> V2: moves the check to the counting loop as Jakub suggested, > alters the commit message accordingly. > > net/core/bpf_sk_storage.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/net/core/bpf_sk_storage.c b/net/core/bpf_sk_storage.c > index d4172534dfa8..cca7594be92e 100644 > --- a/net/core/bpf_sk_storage.c > +++ b/net/core/bpf_sk_storage.c > @@ -496,8 +496,11 @@ bpf_sk_storage_diag_alloc(const struct nlattr *nla_stgs) > return ERR_PTR(-EPERM); > > nla_for_each_nested(nla, nla_stgs, rem) { > - if (nla_type(nla) == SK_DIAG_BPF_STORAGE_REQ_MAP_FD) > + if (nla_type(nla) == SK_DIAG_BPF_STORAGE_REQ_MAP_FD) { > + if (nla_len(nla) != sizeof(u32)) Jakub, it seems like Lin adds this check to all nla_for_each_nested() loops. IMHO, the better change will be to change nla_for_each_nested() skip empty/not valid NLAs. Thanks > + return ERR_PTR(-EINVAL); > nr_maps++; > + } > } > > diag = kzalloc(struct_size(diag, maps, nr_maps), GFP_KERNEL); > -- > 2.17.1 > >
Hello Leon, > > Jakub, it seems like Lin adds this check to all nla_for_each_nested() loops. > IMHO, the better change will be to change nla_for_each_nested() skip empty/not valid NLAs. > > Thanks I guess you just get these fixes misunderstood. I do not add the nla_len check to **all nla_for_each_nested** :(. I only add checks to those who do not access the attributes without verifying the length, which is buggy. The others, either do a similar nla_len check already or just do nla_validate somewhere else. That is to say, they **validate** the relevant attributes. In short, nla_for_each_nested is just a loop macro that iterates the nlattrs, like nla_for_each macro. It is weird for them to do nlattr validation as there could have already been a call to nla_validate to ensure those attributes are correct. That is, for those who do not, a simple nla_len check is the simplest and most efficient choice. Regards Lin
On Tue, Jul 25, 2023 at 01:24:38PM +0800, Lin Ma wrote: > Hello Leon, > > > > > Jakub, it seems like Lin adds this check to all nla_for_each_nested() loops. > > IMHO, the better change will be to change nla_for_each_nested() skip empty/not valid NLAs. > > > > Thanks > > I guess you just get these fixes misunderstood. I do not add the nla_len check > to **all nla_for_each_nested** :(. I only add checks to those who do not access > the attributes without verifying the length, which is buggy. > > The others, either do a similar nla_len check already or just do nla_validate > somewhere else. That is to say, they **validate** the relevant attributes. > > In short, nla_for_each_nested is just a loop macro that iterates the nlattrs, > like nla_for_each macro. It is weird for them to do nlattr validation as there > could have already been a call to nla_validate to ensure those attributes are > correct. That is, for those who do not, a simple nla_len check is the simplest > and most efficient choice. My concern is related to maintainability in long run. Your check adds another layer of cabal knowledge which will be copied/pasted in other places. Thanks > > Regards > Lin
Hello Leon, > > My concern is related to maintainability in long run. Your check adds > another layer of cabal knowledge which will be copied/pasted in other > places. > > Thanks > Yeah, I guess you are right. I guess I should not just *fix* this issue but also think of the maintainability. The very first idea pop into my mind is to complete the necessary nla_policy hence the invalid nlattrs could be rejected at the very first place. Will spend more time on this. Regards Lin
On Tue, 25 Jul 2023 10:33:30 +0800 Lin Ma wrote: > The nla_for_each_nested parsing in function bpf_sk_storage_diag_alloc > does not check the length of the nested attribute. This can lead to an > out-of-attribute read and allow a malformed nlattr (e.g., length 0) to > be viewed as a 4 byte integer. > > This patch adds an additional check when the nlattr is getting counted. > This makes sure the latter nla_get_u32 can access the attributes with > the correct length. > > Fixes: 1ed4d92458a9 ("bpf: INET_DIAG support in bpf_sk_storage") > Suggested-by: Jakub Kicinski <kuba@kernel.org> > Signed-off-by: Lin Ma <linma@zju.edu.cn> Reviewed-by: Jakub Kicinski <kuba@kernel.org> Those who parse manually must do checks manually. It is what it is.
On Tue, 2023-07-25 at 10:33 +0800, Lin Ma wrote: > The nla_for_each_nested parsing in function bpf_sk_storage_diag_alloc > does not check the length of the nested attribute. This can lead to an > out-of-attribute read and allow a malformed nlattr (e.g., length 0) to > be viewed as a 4 byte integer. > > This patch adds an additional check when the nlattr is getting counted. > This makes sure the latter nla_get_u32 can access the attributes with > the correct length. > > Fixes: 1ed4d92458a9 ("bpf: INET_DIAG support in bpf_sk_storage") > Suggested-by: Jakub Kicinski <kuba@kernel.org> > Signed-off-by: Lin Ma <linma@zju.edu.cn> I guess this should go via the ebpf tree, right? Setting the delegate accordingly. Please correct me if I'm wrong. Thanks! /P
On 7/27/23 12:34 AM, Paolo Abeni wrote: > On Tue, 2023-07-25 at 10:33 +0800, Lin Ma wrote: >> The nla_for_each_nested parsing in function bpf_sk_storage_diag_alloc >> does not check the length of the nested attribute. This can lead to an >> out-of-attribute read and allow a malformed nlattr (e.g., length 0) to >> be viewed as a 4 byte integer. >> >> This patch adds an additional check when the nlattr is getting counted. >> This makes sure the latter nla_get_u32 can access the attributes with >> the correct length. >> >> Fixes: 1ed4d92458a9 ("bpf: INET_DIAG support in bpf_sk_storage") >> Suggested-by: Jakub Kicinski <kuba@kernel.org> >> Signed-off-by: Lin Ma <linma@zju.edu.cn> > > I guess this should go via the ebpf tree, right? Setting the delegate > accordingly. Already applied to the bpf tree. Thanks. pw-bot seems not doing auto-reply for the bpf tree.
diff --git a/net/core/bpf_sk_storage.c b/net/core/bpf_sk_storage.c index d4172534dfa8..cca7594be92e 100644 --- a/net/core/bpf_sk_storage.c +++ b/net/core/bpf_sk_storage.c @@ -496,8 +496,11 @@ bpf_sk_storage_diag_alloc(const struct nlattr *nla_stgs) return ERR_PTR(-EPERM); nla_for_each_nested(nla, nla_stgs, rem) { - if (nla_type(nla) == SK_DIAG_BPF_STORAGE_REQ_MAP_FD) + if (nla_type(nla) == SK_DIAG_BPF_STORAGE_REQ_MAP_FD) { + if (nla_len(nla) != sizeof(u32)) + return ERR_PTR(-EINVAL); nr_maps++; + } } diag = kzalloc(struct_size(diag, maps, nr_maps), GFP_KERNEL);
The nla_for_each_nested parsing in function bpf_sk_storage_diag_alloc does not check the length of the nested attribute. This can lead to an out-of-attribute read and allow a malformed nlattr (e.g., length 0) to be viewed as a 4 byte integer. This patch adds an additional check when the nlattr is getting counted. This makes sure the latter nla_get_u32 can access the attributes with the correct length. Fixes: 1ed4d92458a9 ("bpf: INET_DIAG support in bpf_sk_storage") Suggested-by: Jakub Kicinski <kuba@kernel.org> Signed-off-by: Lin Ma <linma@zju.edu.cn> --- V1 -> V2: moves the check to the counting loop as Jakub suggested, alters the commit message accordingly. net/core/bpf_sk_storage.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)