Re: [PATCH v2 05/21] libmultipath (coverity): improve input checking in parse_vpd_pg83

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Dec 01, 2021 at 01:36:34PM +0100, mwilck@xxxxxxxx wrote:
> From: Martin Wilck <mwilck@xxxxxxxx>
> 
> Check offsets and other obvious errors in the VPD83 data.
> 
> The original reason to do this was to fix "tained scalar"
> warnings from coverity. But this doesn't suffice for coverity
> without using a constant boundary (WWID_SIZE) for "len" in
> parse_vpd_pg80() and for "vpd_len" in parse_vpd_pg83(), even
> though the computed boundaries are tighter than the constant
> ones.
> 
> Signed-off-by: Martin Wilck <mwilck@xxxxxxxx>
Reviewed-by: Benjamin Marzinski <bmarzins@xxxxxxxxxx>
> ---
>  libmultipath/discovery.c | 134 +++++++++++++++++++++++++--------------
>  1 file changed, 88 insertions(+), 46 deletions(-)
> 
> diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
> index 36ea7b3..977aed9 100644
> --- a/libmultipath/discovery.c
> +++ b/libmultipath/discovery.c
> @@ -36,6 +36,8 @@
>  #include "print.h"
>  #include "strbuf.h"
>  
> +#define VPD_BUFLEN 4096
> +
>  struct vpd_vendor_page vpd_vendor_pages[VPD_VP_ARRAY_SIZE] = {
>  	[VPD_VP_UNDEF]	= { 0x00, "undef" },
>  	[VPD_VP_HP3PAR]	= { 0xc0, "hp3par" },
> @@ -1086,6 +1088,8 @@ parse_vpd_pg80(const unsigned char *in, char *out, size_t out_len)
>  	if (out_len == 0)
>  		return 0;
>  
> +	if (len > WWID_SIZE)
> +		len = WWID_SIZE;
>  	/*
>  	 * Strip leading and trailing whitespace
>  	 */
> @@ -1115,84 +1119,123 @@ parse_vpd_pg83(const unsigned char *in, size_t in_len,
>  	const unsigned char *d;
>  	const unsigned char *vpd = NULL;
>  	size_t len, vpd_len, i;
> -	int vpd_type, prio = -1, naa_prio;
> +	int vpd_type, prio = -1;
> +	int err = -ENODATA;
> +
> +	/* Need space at least for one digit */
> +	if (out_len <= 1)
> +		return 0;
>  
>  	d = in + 4;
> -	while (d < in + in_len) {
> +	while (d <= in + in_len - 4) {
> +		bool invalid = false;
> +		int new_prio = -1;
> +
>  		/* Select 'association: LUN' */
> -		if ((d[1] & 0x30) != 0) {
> -			d += d[3] + 4;
> -			continue;
> -		}
> +		if ((d[1] & 0x30) == 0x30) {
> +			invalid = true;
> +			goto next_designator;
> +		} else if ((d[1] & 0x30) != 0x00)
> +			goto next_designator;
> +
>  		switch (d[1] & 0xf) {
> +			unsigned char good_len;
>  		case 0x3:
>  			/* NAA: Prio 5 */
>  			switch (d[4] >> 4) {
>  			case 6:
>  				/* IEEE Registered Extended: Prio 8 */
> -				naa_prio = 8;
> +				new_prio = 8;
> +				good_len = 16;
>  				break;
>  			case 5:
>  				/* IEEE Registered: Prio 7 */
> -				naa_prio = 7;
> +				new_prio = 7;
> +				good_len = 8;
>  				break;
>  			case 2:
>  				/* IEEE Extended: Prio 6 */
> -				naa_prio = 6;
> +				new_prio = 6;
> +				good_len = 8;
>  				break;
>  			case 3:
>  				/* IEEE Locally assigned: Prio 1 */
> -				naa_prio = 1;
> +				new_prio = 1;
> +				good_len = 8;
>  				break;
>  			default:
>  				/* Default: no priority */
> -				naa_prio = -1;
> +				good_len = 0xff;
>  				break;
>  			}
> -			if (prio < naa_prio) {
> -				prio = naa_prio;
> -				vpd = d;
> -			}
> +
> +			invalid = good_len == 0xff || good_len != d[3];
>  			break;
>  		case 0x2:
>  			/* EUI-64: Prio 4 */
> -			if (prio < 4) {
> -				prio = 4;
> -				vpd = d;
> -			}
> +			invalid = (d[3] != 8 && d[3] != 12 && d[3] != 16);
> +			new_prio = 4;
>  			break;
>  		case 0x8:
>  			/* SCSI Name: Prio 3 */
> -			if (memcmp(d + 4, "eui.", 4) &&
> -			    memcmp(d + 4, "naa.", 4) &&
> -			    memcmp(d + 4, "iqn.", 4))
> -				break;
> -			if (prio < 3) {
> -				prio = 3;
> -				vpd = d;
> -			}
> +			invalid = (d[3] < 4 || (memcmp(d + 4, "eui.", 4) &&
> +						memcmp(d + 4, "naa.", 4) &&
> +						memcmp(d + 4, "iqn.", 4)));
> +			new_prio = 3;
>  			break;
>  		case 0x1:
>  			/* T-10 Vendor ID: Prio 2 */
> -			if (prio < 2) {
> -				prio = 2;
> -				vpd = d;
> -			}
> +			invalid = (d[3] < 8);
> +			new_prio = 2;
>  			break;
> +		case 0xa:
> +			condlog(2, "%s: UUID identifiers not yet supported",
> +				__func__);
> +			break;
> +		default:
> +			invalid = true;
> +			break;
> +		}
> +
> +	next_designator:
> +		if (d + d[3] + 4 - in > (ssize_t)in_len) {
> +			condlog(2, "%s: device descriptor length overflow: %zd > %zu",
> +				__func__, d + d[3] + 4 - in, in_len);
> +			err = -EOVERFLOW;
> +			break;
> +		} else if (invalid) {
> +			condlog(2, "%s: invalid device designator at offset %zd: %02x%02x%02x%02x",
> +				__func__, d - in, d[0], d[1], d[2], d[3]);
> +			/*
> +			 * We checked above that the next offset is within limits.
> +			 * Proceed, fingers crossed.
> +			 */
> +			err = -EINVAL;
> +		} else if (new_prio > prio) {
> +			vpd = d;
> +			prio = new_prio;
>  		}
>  		d += d[3] + 4;
>  	}
>  
>  	if (prio <= 0)
> -		return -ENODATA;
> -	/* Need space at least for one digit */
> -	else if (out_len <= 1)
> -		return 0;
> +		return err;
> +
> +	if (d != in + in_len)
> +		/* Should this be fatal? (overflow covered above) */
> +		condlog(2, "%s: warning: last descriptor end %zd != VPD length %zu",
> +			__func__, d - in, in_len);
>  
>  	len = 0;
>  	vpd_type = vpd[1] & 0xf;
>  	vpd_len = vpd[3];
>  	vpd += 4;
> +	/* untaint vpd_len for coverity */
> +	if (vpd_len > WWID_SIZE) {
> +		condlog(1, "%s: suspicious designator length %zu truncated to %u",
> +			__func__, vpd_len, WWID_SIZE);
> +		vpd_len = WWID_SIZE;
> +	}
>  	if (vpd_type == 0x2 || vpd_type == 0x3) {
>  		size_t i;
>  
> @@ -1206,10 +1249,6 @@ parse_vpd_pg83(const unsigned char *in, size_t in_len,
>  		for (i = 0; i < vpd_len; i++)
>  			len += sprintf(out + len,
>  				       "%02x", vpd[i]);
> -	} else if (vpd_type == 0x8 && vpd_len < 4) {
> -		condlog(1, "%s: VPD length %zu too small for designator type 8",
> -			__func__, vpd_len);
> -		return -EINVAL;
>  	} else if (vpd_type == 0x8) {
>  		if (!memcmp("eui.", vpd, 4))
>  			out[0] =  '2';
> @@ -1316,11 +1355,12 @@ parse_vpd_c0_hp3par(const unsigned char *in, size_t in_len,
>  static int
>  get_vpd_sysfs (struct udev_device *parent, int pg, char * str, int maxlen)
>  {
> -	int len, buff_len;
> -	unsigned char buff[4096];
> +	int len;
> +	size_t buff_len;
> +	unsigned char buff[VPD_BUFLEN];
>  
> -	memset(buff, 0x0, 4096);
> -	if (!parent || sysfs_get_vpd(parent, pg, buff, 4096) <= 0) {
> +	memset(buff, 0x0, VPD_BUFLEN);
> +	if (!parent || sysfs_get_vpd(parent, pg, buff, VPD_BUFLEN) <= 0) {
>  		condlog(3, "failed to read sysfs vpd pg%02x", pg);
>  		return -EINVAL;
>  	}
> @@ -1331,8 +1371,10 @@ get_vpd_sysfs (struct udev_device *parent, int pg, char * str, int maxlen)
>  		return -ENODATA;
>  	}
>  	buff_len = get_unaligned_be16(&buff[2]) + 4;
> -	if (buff_len > 4096)
> +	if (buff_len > VPD_BUFLEN) {
>  		condlog(3, "vpd pg%02x page truncated", pg);
> +		buff_len = VPD_BUFLEN;
> +	}
>  
>  	if (pg == 0x80)
>  		len = parse_vpd_pg80(buff, str, maxlen);
> @@ -1376,7 +1418,7 @@ bool
>  is_vpd_page_supported(int fd, int pg)
>  {
>  	int i, len;
> -	unsigned char buff[4096];
> +	unsigned char buff[VPD_BUFLEN];
>  
>  	len = fetch_vpd_page(fd, 0x00, buff, sizeof(buff));
>  	if (len < 0)
> @@ -1392,7 +1434,7 @@ int
>  get_vpd_sgio (int fd, int pg, int vend_id, char * str, int maxlen)
>  {
>  	int len, buff_len;
> -	unsigned char buff[4096];
> +	unsigned char buff[VPD_BUFLEN];
>  
>  	buff_len = fetch_vpd_page(fd, pg, buff, sizeof(buff));
>  	if (buff_len < 0)
> -- 
> 2.33.1

--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/dm-devel




[Index of Archives]     [DM Crypt]     [Fedora Desktop]     [ATA RAID]     [Fedora Marketing]     [Fedora Packaging]     [Fedora SELinux]     [Yosemite Discussion]     [KDE Users]     [Fedora Docs]

  Powered by Linux