| Message ID | 1535515447-21167-1-git-send-email-asmadeus@codewreck.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | proc/kcore: fix invalid memory access in multi-page read optimization | expand |
On Wed, Aug 29, 2018 at 06:04:07AM +0200, Dominique Martinet wrote: > The 'm' kcore_list item can point to kclist_head, and it is incorrect to > look at m->addr / m->size in this case. > There is no choice but to run through the list of entries for every address > if we did not find any entry in the previous iteration > > Fixes: bf991c2231117 ("proc/kcore: optimize multiple page reads") > Signed-off-by: Dominique Martinet <asmadeus@codewreck.org> > --- > > I guess now I'm looking at bf991c2231117 again that it would be slightly > more efficient to remove the !m check and initialize m to point to > kclist_head like this: > m = list_entry(&kclist_head, struct kcore_list, list); > but it feels a bit forced to me; deferring the choice to others. Good catch! Sorry I missed this last week, Google decided this was spam for some reason. How about fixing it like this? One less conditional in the common case, no hacky list_entry :) diff --git a/fs/proc/kcore.c b/fs/proc/kcore.c index ad72261ee3fe..578926032880 100644 --- a/fs/proc/kcore.c +++ b/fs/proc/kcore.c @@ -464,6 +464,7 @@ read_kcore(struct file *file, char __user *buffer, size_t buflen, loff_t *fpos) ret = -EFAULT; goto out; } + m = NULL; } else if (m->type == KCORE_VMALLOC) { vread(buf, (char *)start, tsz); /* we have to zero-fill user buffer even if no read */ > fs/proc/kcore.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/fs/proc/kcore.c b/fs/proc/kcore.c > index ad72261ee3fe..50036f6e1f52 100644 > --- a/fs/proc/kcore.c > +++ b/fs/proc/kcore.c > @@ -451,7 +451,8 @@ read_kcore(struct file *file, char __user *buffer, size_t buflen, loff_t *fpos) > * If this is the first iteration or the address is not within > * the previous entry, search for a matching entry. > */ > - if (!m || start < m->addr || start >= m->addr + m->size) { > + if (!m || &m->list == &kclist_head || start < m->addr || > + start >= m->addr + m->size) { > list_for_each_entry(m, &kclist_head, list) { > if (start >= m->addr && > start < m->addr + m->size) > -- > 2.17.1 >
Omar Sandoval wrote on Tue, Sep 04, 2018: > On Wed, Aug 29, 2018 at 06:04:07AM +0200, Dominique Martinet wrote: > > The 'm' kcore_list item can point to kclist_head, and it is incorrect to > > look at m->addr / m->size in this case. > > There is no choice but to run through the list of entries for every address > > if we did not find any entry in the previous iteration > > > > Fixes: bf991c2231117 ("proc/kcore: optimize multiple page reads") > > Signed-off-by: Dominique Martinet <asmadeus@codewreck.org> > > --- > > > > I guess now I'm looking at bf991c2231117 again that it would be slightly > > more efficient to remove the !m check and initialize m to point to > > kclist_head like this: > > m = list_entry(&kclist_head, struct kcore_list, list); > > but it feels a bit forced to me; deferring the choice to others. > > Good catch! Sorry I missed this last week, Google decided this was spam > for some reason. Joys of self-hosted emails, it happens from time to time :/ > How about fixing it like this? One less conditional in the common > case, no hacky list_entry :) Good idea, I'll send a v2 in a few minutes after rebooting into it, no reason it won't work but might as well make earth a slightly warmer place along the way.
diff --git a/fs/proc/kcore.c b/fs/proc/kcore.c index ad72261ee3fe..50036f6e1f52 100644 --- a/fs/proc/kcore.c +++ b/fs/proc/kcore.c @@ -451,7 +451,8 @@ read_kcore(struct file *file, char __user *buffer, size_t buflen, loff_t *fpos) * If this is the first iteration or the address is not within * the previous entry, search for a matching entry. */ - if (!m || start < m->addr || start >= m->addr + m->size) { + if (!m || &m->list == &kclist_head || start < m->addr || + start >= m->addr + m->size) { list_for_each_entry(m, &kclist_head, list) { if (start >= m->addr && start < m->addr + m->size)
The 'm' kcore_list item can point to kclist_head, and it is incorrect to look at m->addr / m->size in this case. There is no choice but to run through the list of entries for every address if we did not find any entry in the previous iteration Fixes: bf991c2231117 ("proc/kcore: optimize multiple page reads") Signed-off-by: Dominique Martinet <asmadeus@codewreck.org> --- I guess now I'm looking at bf991c2231117 again that it would be slightly more efficient to remove the !m check and initialize m to point to kclist_head like this: m = list_entry(&kclist_head, struct kcore_list, list); but it feels a bit forced to me; deferring the choice to others. fs/proc/kcore.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)