netdev - Re: [PATCH] r8152: Add support for setting MAC to system's Auxiliary MAC address

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20160602074656.GM29844@pali>
Date:	Thu, 2 Jun 2016 09:46:56 +0200
From:	Pali Rohár <pali.rohar@...il.com>
To:	Mario Limonciello <mario_limonciello@...l.com>
Cc:	hayeswang@...ltek.com, LKML <linux-kernel@...r.kernel.org>,
	Netdev <netdev@...r.kernel.org>,
	Linux USB <linux-usb@...r.kernel.org>,
	anthony.wong@...onical.com
Subject: Re: [PATCH] r8152: Add support for setting MAC to system's Auxiliary
 MAC address

Hi! As ACPI bytecode is untrusted for me and also for running kernel, we
should not expect that it does not contain any bugs or other problems.
So I would propose these checks to prevent something wrong...

On Wednesday 01 June 2016 16:50:44 Mario Limonciello wrote:
> +static void set_auxiliary_addr(struct sockaddr *sa)
> +{
> +	acpi_status status;
> +	acpi_handle handle;
> +	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
> +	union acpi_object *obj;
> +	int i;
> +	char *ptr;
> +
> +	acpi_get_handle(NULL, "\\_SB", &handle);

Check return value of acpi_get_handle

> +	status = acpi_evaluate_object(handle, "AMAC", NULL, &buffer);

This is question for ACPI devs, it is not possible to call directly?

  acpi_evaluate_object(NULL, "\\_SB.AMAC", NULL, &buffer);

And what happen if we try to evaluate objects which do not exist? Does
not it show some warning or error in dmesg about non existent object?
Such errors should be silent here.

> +	obj = (union acpi_object *)buffer.pointer;

Check buffer.type

> +	if (ACPI_SUCCESS(status) && (obj->string.length == 0x17)) {
> +		/* returns _AUXMAC_#AABBCCDDEEFF#
> +		 * this pulls out _AUXMAC# from start and # from end
> +		 */
> +		ptr = obj->string.pointer + 9;

Verify that string really contains that _AUXMAX# prefix. This is really
obscure and nonstandard format for specifying MAC address and in my
opinion it should be properly checked. Nonstandard formats can be
changed in future and we could have problems.

> +		pr_info("r8152: Using system auxiliary MAC address");

It would be great to write also mac address into that pr_info

> +		for (i = 0; i < 6; i++, ptr += 2)
> +			sa->sa_data[i] = amac_ascii_to_hex(*ptr) << 4 |
> +					 amac_ascii_to_hex(*(ptr + 1));
> +	}

In case of some acpi check fails throw warning (or error).

And there is memory leak, you allocated buffer with ACPI_ALLOCATE_BUFFER
but you did not free it.

> +}

And my last question is: Are really all Dell docks comes with this one
realtek chip? I'm pessimist in this, because I see how other components
(like HDD vendor, touchpad type, smardcard chips, motherboards, display
panels, wifi chips) can be different in two laptops of same Dell model.

-- 
Pali Rohár
pali.rohar@...il.com