Hi, Lianbo
For the checking condition, I would recommend using the following
methods, what do you think?
+ if (size != SAFE_NETDUMP_ELF_HEADER_SIZE &&
+ size != MIN_NETDUMP_ELF_HEADER_SIZE) {
I agrre with Kazu, if use "size !=
MIN_NETDUMP_ELF_HEADER_SIZE/SAFE_NETDUMP_ELF_HEADER_SIZE" this issue can not be
fixed, size is equal to or more than MIN_NETDUMP_ELF_HEADER_SIZE is valid.
In addition, would you mind updating another error output in the
is_netdump()? For example:
size = SAFE_NETDUMP_ELF_HEADER_SIZE;
if ((eheader = (char *)malloc(size)) == NULL) {
- fprintf(stderr, "cannot malloc minimum ELF header
buffer\n");
+ fprintf(stderr, "cannot malloc ELF header buffer\n");
clean_exit(1);
}
Ok.
How about below patch set?
From 5ec6ac06ba7fd32e96463a54ebc3f733f1054a90 Mon Sep 17 00:00:00 2001
From: Qianli Zhao <zhaoqianli(a)xiaomi.com>
Date: Mon, 30 Nov 2020 17:17:32 +0800
Subject: [PATCH] netdump: fix regression for tiny kdump files
Commit f42db6a33f0e ("Support core files with "unusual" layout")
increased the minimal file size from MIN_NETDUMP_ELF_HEADER_SIZE to
SAFE_NETDUMP_ELF_HEADER_SIZE which can lead to crash rejecting
raw RAM dumpfiles
Fix that by erroring out only if we get less than
MIN_NETDUMP_ELF_HEADER_SIZE bytes.
Signed-off-by: Qianli Zhao <zhaoqianli(a)xiaomi.com>
---
netdump.c | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/netdump.c b/netdump.c
index c76d9dd..ca9b459 100644
--- a/netdump.c
+++ b/netdump.c
@@ -119,7 +119,8 @@ is_netdump(char *file, ulong source_query)
Elf64_Phdr *load64;
char *eheader, *sect0;
char buf[BUFSIZE];
- size_t size, len, tot;
+ ssize_t size;
+ size_t len, tot;
Elf32_Off offset32;
Elf64_Off offset64;
ulong format;
@@ -134,7 +135,7 @@ is_netdump(char *file, ulong source_query)
size = SAFE_NETDUMP_ELF_HEADER_SIZE;
if ((eheader = (char *)malloc(size)) == NULL) {
- fprintf(stderr, "cannot malloc minimum ELF header buffer\n");
+ fprintf(stderr, "cannot malloc ELF header buffer\n");
clean_exit(1);
}
@@ -142,10 +143,14 @@ is_netdump(char *file, ulong source_query)
if (!read_flattened_format(fd, 0, eheader, size))
goto bailout;
} else {
- if (read(fd, eheader, size) != size) {
+ size = read(fd, eheader, size);
+ if (size < 0) {
sprintf(buf, "%s: ELF header read", file);
perror(buf);
goto bailout;
+ } else if (size < MIN_NETDUMP_ELF_HEADER_SIZE) {
+ fprintf(stderr, "%s: file too small!\n", file);
+ goto bailout;
}
}
--
2.7.4
________________________________
From: HAGIO KAZUHITO(�尾 一仁) <k-hagio-ab(a)nec.com>
Sent: Tuesday, December 22, 2020 9:14
To: Discussion list for crash utility usage, maintenance and development
Cc: 赵乾利
Subject: [External Mail]RE: [Crash-utility] [PATCH V2] netdump: fix regression for tiny
kdump files
*外部邮件,谨慎处理 | This message originated from outside of XIAOMI. Please treat this email with
caution*
Hi Lianbo,
> -----Original Message-----
> From: crash-utility-bounces(a)redhat.com <crash-utility-bounces(a)redhat.com> On
Behalf Of lijiang
> Sent: Monday, December 21, 2020 11:42 PM
> To: crash-utility(a)redhat.com; zhaoqianli(a)xiaomi.com
> Subject: Re: [Crash-utility] [PATCH V2] netdump: fix regression for tiny kdump
files
>
> Hi, Qianli
>
> Thanks for the patch.
> 在 2020年12月01日 14:45, crash-utility-request(a)redhat.com 写道:
> > Date: Tue, 1 Dec 2020 10:56:02 +0800
> > From: Qianli Zhao <zhaoqianligood(a)gmail.com>
> > To: crash-utility(a)redhat.com, minipli(a)grsecurity.net
> > Subject: [Crash-utility] [PATCH V2] netdump: fix regression for tiny
> > kdump files
> > Message-ID:
> > <1606791362-5604-1-git-send-email-zhaoqianligood(a)gmail.com>
> > Content-Type: text/plain; charset="US-ASCII"
> >
> > From: Qianli Zhao <zhaoqianli(a)xiaomi.com>
> >
> > Commit f42db6a33f0e ("Support core files with "unusual"
layout")
> > increased the minimal file size from MIN_NETDUMP_ELF_HEADER_SIZE to
> > SAFE_NETDUMP_ELF_HEADER_SIZE which lead to crash rejecting very
> > small kdump files.
> >
> Good findings.
>
> > Fix that by erroring out only if we get less than
> > MIN_NETDUMP_ELF_HEADER_SIZE bytes.
> >
> > Signed-off-by: Qianli Zhao <zhaoqianli(a)xiaomi.com>
> > ---
> > - Update commit message
> > - Add more accurate judgment of read() return value
> > ---
> > netdump.c | 9 +++++++--
> > 1 file changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/netdump.c b/netdump.c
> > index c76d9dd..9a36931 100644
> > --- a/netdump.c
> > +++ b/netdump.c
> > @@ -119,7 +119,8 @@ is_netdump(char *file, ulong source_query)
> > Elf64_Phdr *load64;
> > char *eheader, *sect0;
> > char buf[BUFSIZE];
> > - size_t size, len, tot;
> > + ssize_t size;
> > + size_t len, tot;
> > Elf32_Off offset32;
> > Elf64_Off offset64;
> > ulong format;
> > @@ -142,10 +143,14 @@ is_netdump(char *file, ulong source_query)
> > if (!read_flattened_format(fd, 0, eheader, size))
> > goto bailout;
> > } else {
> > - if (read(fd, eheader, size) != size) {
> > + size = read(fd, eheader, size);
> > + if (size < 0) {
> > sprintf(buf, "%s: ELF header read", file);
> > perror(buf);
> > goto bailout;
> > + } else if (size < MIN_NETDUMP_ELF_HEADER_SIZE) {
>
For the checking condition, I would recommend using the following
methods, what do you think?
+ if (size != SAFE_NETDUMP_ELF_HEADER_SIZE &&
+ size != MIN_NETDUMP_ELF_HEADER_SIZE) {
> sprintf(buf, "%s: ELF header read", file);
> perror(buf);
> goto bailout;
> }
Do you mean putting "size < 0" and "size < MIN_NETDUMP_ELF_HEADER
SIZE"
together? I think it would be good to separate an read error and a format
error for better debugging.
And according to ramdump_to_elf(), the size of an ELF header from a RAM
dumpfile varies depending on the number of nodes, and is equal to or more
than MIN_NETDUMP_ELF_HEADER_SIZE if valid. Actually, the value that Qianli
showed before was 232 [1], which is not either SAFE_NETDUMP_ELF_HEADER_SIZE(304)
or MIN_NETDUMP_ELF_HEADER_SIZE(176).
[1]
https://www.redhat.com/archives/crash-utility/2020-November/msg00080.html
Thanks,
Kazu
>
>
In addition, would you mind updating another error output in the
is_netdump()? For example:
size = SAFE_NETDUMP_ELF_HEADER_SIZE;
if ((eheader = (char *)malloc(size)) == NULL) {
- fprintf(stderr, "cannot malloc minimum ELF header
buffer\n");
+ fprintf(stderr, "cannot malloc ELF header buffer\n");
clean_exit(1);
}
>
> Thanks.
> Lianbo
>
> > + fprintf(stderr, "%s: file too small!\n", file);
> > + goto bailout;
> > }
> > }
> >
> > -- 2.7.4
>
> --
> Crash-utility mailing list
> Crash-utility(a)redhat.com
>
https://www.redhat.com/mailman/listinfo/crash-utility
#/******本邮件及其附件含有小米公司的保密信息,仅限于发送给上面地址中列出的个人或群组。禁止任何其他人以任何形式使用(包括但不限于全部或部分地泄露、复制、或散发)本邮件中的信息。如果您错收了本邮件,请您立即电话或邮件通知发件人并删除本邮件!
This e-mail and its attachments contain confidential information from XIAOMI, which is
intended only for the person or entity whose address is listed above. Any use of the
information contained herein in any way (including, but not limited to, total or partial
disclosure, reproduction, or dissemination) by persons other than the intended
recipient(s) is prohibited. If you receive this e-mail in error, please notify the sender
by phone or email immediately and delete it!******/#