| Message ID | 20180902154443.4776-1-cgxu519@gmx.com |
|---|---|
| State | Rejected |
| Delegated to: | Richard Weinberger |
| Headers | show |
| Series | jffs2: add additinal sanity check for jffs2_acl_from_medium() | expand |
On Sun, Sep 2, 2018 at 5:45 PM Chengguang Xu <cgxu519@gmx.com> wrote: > > In the case ACL_USER and ACL_GROUP we check if value has exceeded end, > add same check in the case ACL_OTHER as well. Did you hit a problem in that area or was this found by review? From looking at the code I'd say it is fine as is. In the ACL_MASK/_OTHER case we don't look into the entry object like ACL_USER/_GROUP do, we immediately break the switch and run another round in the for loop. And here we do: entry = value; if (value + sizeof(struct jffs2_acl_entry_short) > end) goto fail; Which is what your additional check does. So, we'd check twice. What do I miss? > Signed-off-by: Chengguang Xu <cgxu519@gmx.com> > --- > fs/jffs2/acl.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/fs/jffs2/acl.c b/fs/jffs2/acl.c > index 093ffbd82395..fce32de3a18f 100644 > --- a/fs/jffs2/acl.c > +++ b/fs/jffs2/acl.c > @@ -94,6 +94,8 @@ static struct posix_acl *jffs2_acl_from_medium(void *value, size_t size) > case ACL_MASK: > case ACL_OTHER: > value += sizeof(struct jffs2_acl_entry_short); > + if (value > end) > + goto fail; > break; > > case ACL_USER: > -- > 2.17.1 > > > ______________________________________________________ > Linux MTD discussion mailing list > http://lists.infradead.org/mailman/listinfo/linux-mtd/
On 09/03/2018 04:45 PM, Richard Weinberger wrote: > On Sun, Sep 2, 2018 at 5:45 PM Chengguang Xu <cgxu519@gmx.com> wrote: >> In the case ACL_USER and ACL_GROUP we check if value has exceeded end, >> add same check in the case ACL_OTHER as well. > Did you hit a problem in that area or was this found by review? > From looking at the code I'd say it is fine as is. > In the ACL_MASK/_OTHER case we don't look into the entry object like > ACL_USER/_GROUP > do, we immediately break the switch and run another round in the for loop. > And here we do: > entry = value; > if (value + sizeof(struct jffs2_acl_entry_short) > end) > goto fail; > > Which is what your additional check does. So, we'd check twice. > What do I miss? You are right, it is actually not needed. Sorry, please just drop the patch.
Am Montag, 3. September 2018, 15:28:31 CEST schrieb cgxu519: > On 09/03/2018 04:45 PM, Richard Weinberger wrote: > > On Sun, Sep 2, 2018 at 5:45 PM Chengguang Xu <cgxu519@gmx.com> wrote: > >> In the case ACL_USER and ACL_GROUP we check if value has exceeded end, > >> add same check in the case ACL_OTHER as well. > > Did you hit a problem in that area or was this found by review? > > From looking at the code I'd say it is fine as is. > > In the ACL_MASK/_OTHER case we don't look into the entry object like > > ACL_USER/_GROUP > > do, we immediately break the switch and run another round in the for loop. > > And here we do: > > entry = value; > > if (value + sizeof(struct jffs2_acl_entry_short) > end) > > goto fail; > > > > Which is what your additional check does. So, we'd check twice. > > What do I miss? > > You are right, it is actually not needed. Sorry, please just drop the patch. No problem, that's why we have a review process. :-) Thanks, //richard
diff --git a/fs/jffs2/acl.c b/fs/jffs2/acl.c index 093ffbd82395..fce32de3a18f 100644 --- a/fs/jffs2/acl.c +++ b/fs/jffs2/acl.c @@ -94,6 +94,8 @@ static struct posix_acl *jffs2_acl_from_medium(void *value, size_t size) case ACL_MASK: case ACL_OTHER: value += sizeof(struct jffs2_acl_entry_short); + if (value > end) + goto fail; break; case ACL_USER:
In the case ACL_USER and ACL_GROUP we check if value has exceeded end, add same check in the case ACL_OTHER as well. Signed-off-by: Chengguang Xu <cgxu519@gmx.com> --- fs/jffs2/acl.c | 2 ++ 1 file changed, 2 insertions(+)