[PATCH v2] ruckus_fw_header: Add Ruckus Firmware builder for Ruckus R500 and others

[Rafał Miłecki]

On 3.12.2021 01:18, tusker at tusker.org wrote:
> From: Damien Mascord <tusker at tusker.org>
> 
> This image builder builds a tftp updatable binary for Ruckus hardware

Hey, I missed this patch, my suggestion would be to use something like
[PATCH firmware-utils V2]

Let me look at it now.

First git complains about some whitespaces:

Applying: ruckus_fw_header: Add Ruckus Firmware builder for Ruckus R500 and others
.git/rebase-apply/patch:97: trailing whitespace.
         char load_address[4] = {
.git/rebase-apply/patch:98: trailing whitespace.
                 0x80, 0x06, 0x00, 0x00 };
.git/rebase-apply/patch:110: trailing whitespace.
                 0x4f, 0x50, 0x45, 0x4e, 0x57, 0x52, 0x54, 0x31, 0x32, 0x33, 0x34, 0x35,
.git/rebase-apply/patch:118: trailing whitespace.
         char header4[4] = {
.git/rebase-apply/patch:120: trailing whitespace.
         char entry_point[4] = {
warning: squelched 13 whitespace errors
warning: 18 lines add whitespace errors.


> @@ -0,0 +1,258 @@
> +/*
> + * rucks_fw_header.c - partially based on OpenWrt's xorimage.c
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
> + */

Please use SPDX header


> +int write_header(FILE *out, char* md5sum){
> +	// lzma kernel offset is ruckus header + uimage header = 160 + 64 = 224 = 0xE0
> +	char header1[12] = {
> +		0x52, 0x43, 0x4b, 0x53, 0x00, 0x13, 0x00, 0x00, 0x00, 0xe0, 0x6c, 0x37 };
> +	char load_address[4] = {	
> +		0x80, 0x06, 0x00, 0x00 };	
> +	char fakesize[4] = {
> +		0x01, 0x76, 0xd7, 0x5c };
> +	char header_postsize[4] = {
> +		0x60, 0x9a, 0x43, 0xd2 };
> +	char fakemd5[16] = {
> +		0x39, 0x17, 0x73, 0x56, 0xed, 0x87, 0x33, 0x9a, 0xe3, 0xe4, 0xff, 0xc9,
> +		0xee, 0xcd, 0xd2, 0x9c };
> +	char header2[4] = {
> +		0x00, 0x04, 0x66, 0x69 };
> +	//200.7.10.202.9 - OPENWRT12345678
> +	char fake_version[16] = {
> +		0x4f, 0x50, 0x45, 0x4e, 0x57, 0x52, 0x54, 0x31, 0x32, 0x33, 0x34, 0x35,
> +		0x36, 0x37, 0x38, 0x39 };
> +	char header3[48] = {
> +		0x00, 0x01, 0x03, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> +		0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> +		0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> +		0x7a, 0x66, 0x37, 0x37, 0x35, 0x32, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 };
> +	//200.7.10.202.9 - OPENWRT12345678
> +	char header4[4] = {	
> +		0x00, 0x00, 0x00, 0x03 };
> +	char entry_point[4] = {	
> +		0x80, 0x06, 0x00, 0x00 };
> +	char header_postentry[28] = {
> +	    0x00, 0x00, 0x00, 0x01, 0x01, 0x76, 0xcf, 0x60, 0x00, 0x00, 0x00, 0x00,
> +	    0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> +	    0x00, 0x00, 0x00, 0x00 };

It's full of magic values, hard to maintain & hacky. Please use a packed
struct with proper width (u8 / u16 / u32) named fields. Fill them using
real values and helpers like cpu_to_le32 / cpu_to_be32.

Then fwrite whole struct at once.

Make function static.


> +void usage(void) __attribute__ (( __noreturn__ ));

Do we need that?


> +void usage(void)
> +{
> +	fprintf(stderr, "Usage: ruckus_fw_header [-i infile] [-o outfile] [-d]\n");
> +	exit(EXIT_FAILURE);
> +}

Static function


> +int main(int argc, char **argv)
> +{
> +	char buf[1];	/* keep this at 1k or adjust garbage calc below */
> +	FILE *in = stdin;
> +	FILE *out = stdout;
> +	char *ifn = NULL;
> +	char *ofn = NULL;
> +	int c;
> +	int v0, v1, v2;
> +	size_t n;
> +	int p_len, p_off = 0;
> +    uint8_t  md5sum[0x10];

Use tab for indent.


> +
> +	while ((c = getopt(argc, argv, "i:o:h")) != -1) {
> +		switch (c) {
> +			case 'i':
> +				ifn = optarg;
> +				break;
> +			case 'o':
> +				ofn = optarg;
> +				break;
> +			case 'h':
> +			default:
> +				usage();
> +		}
> +	}
> +	
> +	data_size = get_file_size(ifn);

Check returned value.
Maybe make that a local variable to keep code a bit cleaner.


> +	if (optind != argc || optind == 1) {
> +		fprintf(stderr, "illegal arg \"%s\"\n", argv[optind]);
> +		usage();
> +	}
> +
> +	if (ifn && md5_file(ifn, md5sum)) {
> +		fprintf(stderr, "can not get md5sum for \"%s\"\n", ifn);
> +		usage();
> +	}
> +
> +	if (ifn && !(in = fopen(ifn, "r"))) {
> +		fprintf(stderr, "can not open \"%s\" for reading\n", ifn);
> +		usage();
> +	}
> +
> +	if (ofn && !(out = fopen(ofn, "w"))) {
> +		fprintf(stderr, "can not open \"%s\" for writing\n", ofn);
> +		usage();
> +	}
> +
> +	if (ofn && (write_header(out, md5sum))) {
> +		fprintf(stderr, "can not write header to \"%s\"\n", ofn);
> +		usage();
> +	}
> +
> +	while ((n = fread(buf, 1, sizeof(buf), in)) > 0) {
> +		if (n < sizeof(buf)) {
> +			if (ferror(in)) {
> +			FREAD_ERROR:
> +				fprintf(stderr, "fread error\n");
> +				return EXIT_FAILURE;
> +			}
> +		}
> +
> +		if (!fwrite(buf, n, 1, out)) {
> +		FWRITE_ERROR:
> +			fprintf(stderr, "fwrite error\n");
> +			return EXIT_FAILURE;
> +		}
> +	}
> +
> +	if (ferror(in)) {
> +		goto FREAD_ERROR;
> +	}
> +
> +	if (fflush(out)) {
> +		goto FWRITE_ERROR;
> +	}

It's much more common to keep failure path at the end of function.
Jumping into a conditional block inside a loop looks a bit hacky - even
if just to exit.

It also results in printing "fwrite error" on fflush() failure. A bit
misleading.


> +
> +	fclose(in);
> +	fclose(out);
> +
> +	return EXIT_SUCCESS;
> +}