[Crash-utility] [PATCH V2] netdump: fix regression for tiny kdump files

[HAGIO KAZUHITO(萩尾 一仁)]

Hi Lianbo,

> -----Original Message-----
> From: crash-utility-bounces at redhat.com <crash-utility-bounces at redhat.com> On Behalf Of lijiang
> Sent: Monday, December 21, 2020 11:42 PM
> To: crash-utility at redhat.com; zhaoqianli at 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 at redhat.com 写道:
> > Date: Tue,  1 Dec 2020 10:56:02 +0800
> > From: Qianli Zhao <zhaoqianligood at gmail.com>
> > To: crash-utility at redhat.com, minipli at grsecurity.net
> > Subject: [Crash-utility] [PATCH V2] netdump: fix regression for tiny
> > 	kdump	files
> > Message-ID:
> > 	<1606791362-5604-1-git-send-email-zhaoqianligood at gmail.com>
> > Content-Type: text/plain; charset="US-ASCII"
> >
> > From: Qianli Zhao <zhaoqianli at 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 at 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 at redhat.com
> https://www.redhat.com/mailman/listinfo/crash-utility