linux-kernel - Re: [PATCH v2 01/27] staging: unisys: visorbus change -1 return values

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]

Date:	Wed, 1 Jun 2016 09:27:52 -0400
From:	Neil Horman <nhorman@...hat.com>
To:	David Kershner <david.kershner@...sys.com>
Cc:	corbet@....net, tglx@...utronix.de, mingo@...hat.com,
	hpa@...or.com, gregkh@...uxfoundation.org,
	erik.arfvidson@...sys.com, timothy.sell@...sys.com,
	hofrat@...dl.org, dzickus@...hat.com, jes.sorensen@...hat.com,
	alexander.curtin@...sys.com, janani.rvchndrn@...il.com,
	sudipm.mukherjee@...il.com, prarit@...hat.com,
	david.binder@...sys.com, dan.j.williams@...el.com,
	linux-kernel@...r.kernel.org, linux-doc@...r.kernel.org,
	driverdev-devel@...uxdriverproject.org, sparmaintainer@...sys.com
Subject: Re: [PATCH v2 01/27] staging: unisys: visorbus change -1 return
 values

On Tue, May 31, 2016 at 10:26:27PM -0400, David Kershner wrote:
> From: Erik Arfvidson <erik.arfvidson@...sys.com>
> 
> This patch changes the vague -1 return values to -EFAULT since
> it would be the most appropriate, given that this error
> would only occur in an unexpected bad offset field.
> Resulting in a bad address.
> 
> Signed-off-by: Erik Arfvidson <erik.arfvidson@...sys.com>
> Signed-off-by: David Kershner <david.kershner@...sys.com>
> Reviewed-by: Tim Sell <Timothy.Sell@...sys.com>
> ---
>  drivers/staging/unisys/visorbus/visorbus_main.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/staging/unisys/visorbus/visorbus_main.c b/drivers/staging/unisys/visorbus/visorbus_main.c
> index 3a147db..d32b898 100644
> --- a/drivers/staging/unisys/visorbus/visorbus_main.c
> +++ b/drivers/staging/unisys/visorbus/visorbus_main.c
> @@ -876,10 +876,10 @@ write_vbus_chp_info(struct visorchannel *chan,
>  	int off = sizeof(struct channel_header) + hdr_info->chp_info_offset;
>  
>  	if (hdr_info->chp_info_offset == 0)
> -		return -1;
> +		return -EFAULT;
>  
>  	if (visorchannel_write(chan, off, info, sizeof(*info)) < 0)
> -		return -1;
> +		return -EFAULT;
>  	return 0;
>  }
>  
> @@ -895,10 +895,10 @@ write_vbus_bus_info(struct visorchannel *chan,
>  	int off = sizeof(struct channel_header) + hdr_info->bus_info_offset;
>  
>  	if (hdr_info->bus_info_offset == 0)
> -		return -1;
> +		return -EFAULT;
>  
>  	if (visorchannel_write(chan, off, info, sizeof(*info)) < 0)
> -		return -1;
> +		return -EFAULT;
>  	return 0;
>  }
>  
> @@ -915,10 +915,10 @@ write_vbus_dev_info(struct visorchannel *chan,
>  	    (hdr_info->device_info_struct_bytes * devix);
>  
>  	if (hdr_info->dev_info_offset == 0)
> -		return -1;
> +		return -EFAULT;
>  
>  	if (visorchannel_write(chan, off, info, sizeof(*info)) < 0)
> -		return -1;
> +		return -EFAULT;
>  	return 0;
>  }
>  
> -- 
> 1.9.1
> 

This seems fine, but why are you bothering to return anything at all, since the
return code is ignored at all the call sites.  Or more directly, why aren't you
checking these return codes, and acting appropriately if a fault is returned?

Neil