Hi, On 20-06-20, Baptiste Jonglez wrote: > I should have added more details in the commit message: this fixes a > serious regression where procd fails to start some services, for instance > rpcd. See FS#3177.
Any feedback on this regression fix?
Thanks,
Baptiste
> This is the same regression that Rafał found in 19.07.2 and 19.07.3.
>
> I would like a review that I backported the right set of commits: I don't
> think I missed one, but I'm not 100% sure that the 4 commits I backported
> are all strictly necessary.
>
> In any case, they are all clean cherry-picks.
>
> On 13-06-20, Baptiste Jonglez wrote:
> > From: Baptiste Jonglez <g...@bitsofnetworks.org>
> >
> > Fixes: FS#3177
> > Cc: Felix Fietkau <n...@nbd.name>
> > Cc: Rafał Miłecki <ra...@milecki.pl>
> > Signed-off-by: Baptiste Jonglez <g...@bitsofnetworks.org>
> > ---
> > package/libs/libubox/Makefile | 2 +-
> > ...s-iteration-in-the-blobmsg_check_arr.patch | 75 ++++++++++
> > ...sg-fix-length-in-blobmsg_check_array.patch | 28 ++++
> > ...-and-fix-name-length-checks-in-blobm.patch | 49 +++++++
> > ...21-blobmsg-fix-missing-length-checks.patch | 138 ++++++++++++++++++
> > 5 files changed, 291 insertions(+), 1 deletion(-)
> > create mode 100644
> > package/libs/libubox/patches/0018-blobmsg-fix-attrs-iteration-in-the-blobmsg_check_arr.patch
> > create mode 100644
> > package/libs/libubox/patches/0019-blobmsg-fix-length-in-blobmsg_check_array.patch
> > create mode 100644
> > package/libs/libubox/patches/0020-blobmsg-simplify-and-fix-name-length-checks-in-blobm.patch
> > create mode 100644
> > package/libs/libubox/patches/0021-blobmsg-fix-missing-length-checks.patch
> >
> > diff --git a/package/libs/libubox/Makefile b/package/libs/libubox/Makefile
> > index e3a827c1ab..e4f1a6b503 100644
> > --- a/package/libs/libubox/Makefile
> > +++ b/package/libs/libubox/Makefile
> > @@ -1,7 +1,7 @@
> > include $(TOPDIR)/rules.mk
> >
> > PKG_NAME:=libubox
> > -PKG_RELEASE=4
> > +PKG_RELEASE=5
> >
> > PKG_SOURCE_PROTO:=git
> > PKG_SOURCE_URL=$(PROJECT_GIT)/project/libubox.git
> > diff --git
> > a/package/libs/libubox/patches/0018-blobmsg-fix-attrs-iteration-in-the-blobmsg_check_arr.patch
> >
> > b/package/libs/libubox/patches/0018-blobmsg-fix-attrs-iteration-in-the-blobmsg_check_arr.patch
> > new file mode 100644
> > index 0000000000..2834e10ee3
> > --- /dev/null
> > +++
> > b/package/libs/libubox/patches/0018-blobmsg-fix-attrs-iteration-in-the-blobmsg_check_arr.patch
> > @@ -0,0 +1,75 @@
> > +From 5e75160f48785464f9213c6bc8c72b9372c5318b Mon Sep 17 00:00:00 2001
> > +From: =?UTF-8?q?Rafa=C5=82=20Mi=C5=82ecki?= <ra...@milecki.pl>
> > +Date: Sat, 23 May 2020 13:18:51 +0200
> > +Subject: [PATCH] blobmsg: fix attrs iteration in the
> > blobmsg_check_array_len()
> > +MIME-Version: 1.0
> > +Content-Type: text/plain; charset=UTF-8
> > +Content-Transfer-Encoding: 8bit
> > +
> > +Starting with 75e300aeec25 ("blobmsg: fix wrong payload len passed from
> > +blobmsg_check_array") blobmsg_check_array_len() gets *blob* length
> > +passed as argument. It cannot be used with __blobmsg_for_each_attr()
> > +which expects *data* length.
> > +
> > +Use blobmsg_for_each_attr() which calculates *data* length on its own.
> > +
> > +The same bug was already reported in the past and there was fix attempt
> > +in the commit cd75136b1342 ("blobmsg: fix wrong payload len passed from
> > +blobmsg_check_array"). That change made blobmsg_check_attr_len() calls
> > +fail however.
> > +
> > +This is hopefully the correct & complete fix:
> > +1. blobmsg_check_array_len() gets *blob* length
> > +2. It calls blobmsg_check_attr_len() which requires *blob* length
> > +3. It uses blobmsg_for_each_attr() which gets *data* length
> > +
> > +This fixes iterating over random memory treated as attrs. That was
> > +resulting in check failing randomly for totally correct blobs. It's
> > +critical e.g. for procd project with its instance_fill_array() failing
> > +and procd not starting services.
> > +
> > +Fixes: 75e300aeec25 ("blobmsg: fix wrong payload len passed from
> > blobmsg_check_array")
> > +Signed-off-by: Rafał Miłecki <ra...@milecki.pl>
> > +---
> > + blobmsg.c | 10 ++++++----
> > + 1 file changed, 6 insertions(+), 4 deletions(-)
> > +
> > +diff --git a/blobmsg.c b/blobmsg.c
> > +index 8b9877d..59045e1 100644
> > +--- a/blobmsg.c
> > ++++ b/blobmsg.c
> > +@@ -117,16 +117,18 @@ int blobmsg_check_array(const struct blob_attr
> > *attr, int type)
> > + return blobmsg_check_array_len(attr, type, blob_len(attr));
> > + }
> > +
> > +-int blobmsg_check_array_len(const struct blob_attr *attr, int type,
> > size_t len)
> > ++int blobmsg_check_array_len(const struct blob_attr *attr, int type,
> > ++ size_t blob_len)
> > + {
> > + struct blob_attr *cur;
> > ++ size_t rem;
> > + bool name;
> > + int size = 0;
> > +
> > + if (type > BLOBMSG_TYPE_LAST)
> > + return -1;
> > +
> > +- if (!blobmsg_check_attr_len(attr, false, len))
> > ++ if (!blobmsg_check_attr_len(attr, false, blob_len))
> > + return -1;
> > +
> > + switch (blobmsg_type(attr)) {
> > +@@ -140,11 +142,11 @@ int blobmsg_check_array_len(const struct blob_attr
> > *attr, int type, size_t len)
> > + return -1;
> > + }
> > +
> > +- __blobmsg_for_each_attr(cur, attr, len) {
> > ++ blobmsg_for_each_attr(cur, attr, rem) {
> > + if (type != BLOBMSG_TYPE_UNSPEC && blobmsg_type(cur) != type)
> > + return -1;
> > +
> > +- if (!blobmsg_check_attr_len(cur, name, len))
> > ++ if (!blobmsg_check_attr_len(cur, name, rem))
> > + return -1;
> > +
> > + size++;
> > diff --git
> > a/package/libs/libubox/patches/0019-blobmsg-fix-length-in-blobmsg_check_array.patch
> >
> > b/package/libs/libubox/patches/0019-blobmsg-fix-length-in-blobmsg_check_array.patch
> > new file mode 100644
> > index 0000000000..9db2fb4f9f
> > --- /dev/null
> > +++
> > b/package/libs/libubox/patches/0019-blobmsg-fix-length-in-blobmsg_check_array.patch
> > @@ -0,0 +1,28 @@
> > +From c2fc622b771f679e8f55060ac60cfe02b9a80995 Mon Sep 17 00:00:00 2001
> > +From: Felix Fietkau <n...@nbd.name>
> > +Date: Mon, 25 May 2020 13:44:20 +0200
> > +Subject: [PATCH] blobmsg: fix length in blobmsg_check_array
> > +
> > +blobmsg_check_array_len expects the length of the full attribute buffer,
> > +not just the data length.
> > +Due to other missing length checks (fixed in the next commit), this did
> > +not show up as a test failure
> > +
> > +Signed-off-by: Felix Fietkau <n...@nbd.name>
> > +---
> > + blobmsg.c | 2 +-
> > + 1 file changed, 1 insertion(+), 1 deletion(-)
> > +
> > +diff --git a/blobmsg.c b/blobmsg.c
> > +index 59045e1..daaa9fc 100644
> > +--- a/blobmsg.c
> > ++++ b/blobmsg.c
> > +@@ -114,7 +114,7 @@ bool blobmsg_check_attr_len(const struct blob_attr
> > *attr, bool name, size_t len)
> > +
> > + int blobmsg_check_array(const struct blob_attr *attr, int type)
> > + {
> > +- return blobmsg_check_array_len(attr, type, blob_len(attr));
> > ++ return blobmsg_check_array_len(attr, type, blob_raw_len(attr));
> > + }
> > +
> > + int blobmsg_check_array_len(const struct blob_attr *attr, int type,
> > diff --git
> > a/package/libs/libubox/patches/0020-blobmsg-simplify-and-fix-name-length-checks-in-blobm.patch
> >
> > b/package/libs/libubox/patches/0020-blobmsg-simplify-and-fix-name-length-checks-in-blobm.patch
> > new file mode 100644
> > index 0000000000..a481208789
> > --- /dev/null
> > +++
> > b/package/libs/libubox/patches/0020-blobmsg-simplify-and-fix-name-length-checks-in-blobm.patch
> > @@ -0,0 +1,49 @@
> > +From 639c29d19717616b809d9a1e9042461ab8024370 Mon Sep 17 00:00:00 2001
> > +From: Felix Fietkau <n...@nbd.name>
> > +Date: Mon, 25 May 2020 14:49:35 +0200
> > +Subject: [PATCH] blobmsg: simplify and fix name length checks in
> > + blobmsg_check_name
> > +
> > +blobmsg_hdr_valid_namelen was omitted when name==false
> > +The blob_len vs blobmsg_namelen changes were not taking into account
> > +potential padding between name and data
> > +
> > +Signed-off-by: Felix Fietkau <n...@nbd.name>
> > +---
> > + blobmsg.c | 13 ++++---------
> > + 1 file changed, 4 insertions(+), 9 deletions(-)
> > +
> > +diff --git a/blobmsg.c b/blobmsg.c
> > +index daaa9fc..308bef7 100644
> > +--- a/blobmsg.c
> > ++++ b/blobmsg.c
> > +@@ -48,8 +48,8 @@ static bool blobmsg_hdr_valid_namelen(const struct
> > blobmsg_hdr *hdr, size_t len)
> > +
> > + static bool blobmsg_check_name(const struct blob_attr *attr, size_t len,
> > bool name)
> > + {
> > +- char *limit = (char *) attr + len;
> > + const struct blobmsg_hdr *hdr;
> > ++ uint16_t namelen;
> > +
> > + hdr = blobmsg_hdr_from_blob(attr, len);
> > + if (!hdr)
> > +@@ -58,16 +58,11 @@ static bool blobmsg_check_name(const struct blob_attr
> > *attr, size_t len, bool na
> > + if (name && !hdr->namelen)
> > + return false;
> > +
> > +- if (name && !blobmsg_hdr_valid_namelen(hdr, len))
> > +- return false;
> > +-
> > +- if ((char *) hdr->name + blobmsg_namelen(hdr) + 1 > limit)
> > +- return false;
> > +-
> > +- if (blobmsg_namelen(hdr) > (blob_len(attr) - sizeof(struct
> > blobmsg_hdr)))
> > ++ namelen = blobmsg_namelen(hdr);
> > ++ if (blob_len(attr) < (size_t)blobmsg_hdrlen(namelen))
> > + return false;
> > +
> > +- if (hdr->name[blobmsg_namelen(hdr)] != 0)
> > ++ if (hdr->name[namelen] != 0)
> > + return false;
> > +
> > + return true;
> > diff --git
> > a/package/libs/libubox/patches/0021-blobmsg-fix-missing-length-checks.patch
> > b/package/libs/libubox/patches/0021-blobmsg-fix-missing-length-checks.patch
> > new file mode 100644
> > index 0000000000..bfc440a329
> > --- /dev/null
> > +++
> > b/package/libs/libubox/patches/0021-blobmsg-fix-missing-length-checks.patch
> > @@ -0,0 +1,138 @@
> > +From 66195aee50424cbda0c2d858014e4cc58a2dc029 Mon Sep 17 00:00:00 2001
> > +From: Felix Fietkau <n...@nbd.name>
> > +Date: Mon, 25 May 2020 12:40:04 +0200
> > +Subject: [PATCH] blobmsg: fix missing length checks
> > +
> > +blobmsg_check_attr_len was calling blobmsg_check_data for some, but not all
> > +attribute types. These checks was missing for arrays and tables.
> > +
> > +Additionally, the length check in blobmsg_check_data was a bit off, since
> > +it was comparing the blobmsg data length against the raw blob attr length.
> > +
> > +Fix this by checking the raw blob length against the buffer length in
> > +blobmsg_hdr_from_blob
> > +
> > +Signed-off-by: Felix Fietkau <n...@nbd.name>
> > +---
> > + blobmsg.c | 66 +++++++++++++++++--------------------------------------
> > + 1 file changed, 20 insertions(+), 46 deletions(-)
> > +
> > +diff --git a/blobmsg.c b/blobmsg.c
> > +index 308bef7..7da4183 100644
> > +--- a/blobmsg.c
> > ++++ b/blobmsg.c
> > +@@ -30,31 +30,18 @@ bool blobmsg_check_attr(const struct blob_attr *attr,
> > bool name)
> > + return blobmsg_check_attr_len(attr, name, blob_raw_len(attr));
> > + }
> > +
> > +-static const struct blobmsg_hdr* blobmsg_hdr_from_blob(const struct
> > blob_attr *attr, size_t len)
> > +-{
> > +- if (len < sizeof(struct blob_attr) + sizeof(struct blobmsg_hdr))
> > +- return NULL;
> > +-
> > +- return blob_data(attr);
> > +-}
> > +-
> > +-static bool blobmsg_hdr_valid_namelen(const struct blobmsg_hdr *hdr,
> > size_t len)
> > +-{
> > +- if (len < sizeof(struct blob_attr) + sizeof(struct blobmsg_hdr) +
> > blobmsg_namelen(hdr) + 1)
> > +- return false;
> > +-
> > +- return true;
> > +-}
> > +-
> > +-static bool blobmsg_check_name(const struct blob_attr *attr, size_t len,
> > bool name)
> > ++static bool blobmsg_check_name(const struct blob_attr *attr, bool name)
> > + {
> > + const struct blobmsg_hdr *hdr;
> > + uint16_t namelen;
> > +
> > +- hdr = blobmsg_hdr_from_blob(attr, len);
> > +- if (!hdr)
> > ++ if (!blob_is_extended(attr))
> > ++ return !name;
> > ++
> > ++ if (blob_len(attr) < sizeof(struct blobmsg_hdr))
> > + return false;
> > +
> > ++ hdr = (const struct blobmsg_hdr *)blob_data(attr);
> > + if (name && !hdr->namelen)
> > + return false;
> > +
> > +@@ -68,29 +55,20 @@ static bool blobmsg_check_name(const struct blob_attr
> > *attr, size_t len, bool na
> > + return true;
> > + }
> > +
> > +-static const char* blobmsg_check_data(const struct blob_attr *attr,
> > size_t len, size_t *data_len)
> > +-{
> > +- char *limit = (char *) attr + len;
> > +- const char *data;
> > +-
> > +- *data_len = blobmsg_data_len(attr);
> > +- if (*data_len > blob_raw_len(attr))
> > +- return NULL;
> > +-
> > +- data = blobmsg_data(attr);
> > +- if (data + *data_len > limit)
> > +- return NULL;
> > +-
> > +- return data;
> > +-}
> > +-
> > + bool blobmsg_check_attr_len(const struct blob_attr *attr, bool name,
> > size_t len)
> > + {
> > + const char *data;
> > + size_t data_len;
> > + int id;
> > +
> > +- if (!blobmsg_check_name(attr, len, name))
> > ++ if (len < sizeof(struct blob_attr))
> > ++ return false;
> > ++
> > ++ data_len = blob_raw_len(attr);
> > ++ if (data_len < sizeof(struct blob_attr) || data_len > len)
> > ++ return false;
> > ++
> > ++ if (!blobmsg_check_name(attr, name))
> > + return false;
> > +
> > + id = blob_id(attr);
> > +@@ -100,9 +78,8 @@ bool blobmsg_check_attr_len(const struct blob_attr
> > *attr, bool name, size_t len)
> > + if (!blob_type[id])
> > + return true;
> > +
> > +- data = blobmsg_check_data(attr, len, &data_len);
> > +- if (!data)
> > +- return false;
> > ++ data = blobmsg_data(attr);
> > ++ data_len = blobmsg_data_len(attr);
> > +
> > + return blob_check_type(data, data_len, blob_type[id]);
> > + }
> > +@@ -206,13 +183,13 @@ int blobmsg_parse(const struct blobmsg_policy
> > *policy, int policy_len,
> > + }
> > +
> > + __blob_for_each_attr(attr, data, len) {
> > +- hdr = blobmsg_hdr_from_blob(attr, len);
> > +- if (!hdr)
> > ++ if (!blobmsg_check_attr_len(attr, false, len))
> > + return -1;
> > +
> > +- if (!blobmsg_hdr_valid_namelen(hdr, len))
> > +- return -1;
> > ++ if (!blob_is_extended(attr))
> > ++ continue;
> > +
> > ++ hdr = blob_data(attr);
> > + for (i = 0; i < policy_len; i++) {
> > + if (!policy[i].name)
> > + continue;
> > +@@ -224,9 +201,6 @@ int blobmsg_parse(const struct blobmsg_policy *policy,
> > int policy_len,
> > + if (blobmsg_namelen(hdr) != pslen[i])
> > + continue;
> > +
> > +- if (!blobmsg_check_attr_len(attr, true, len))
> > +- return -1;
> > +-
> > + if (tb[i])
> > + continue;
> _______________________________________________
> openwrt-devel mailing list
> openwrt-devel@lists.openwrt.org
> https://lists.openwrt.org/mailman/listinfo/openwrt-devel
signature.asc
Description: PGP signature
_______________________________________________ openwrt-devel mailing list openwrt-devel@lists.openwrt.org https://lists.openwrt.org/mailman/listinfo/openwrt-devel
