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  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, 13 May 2020 13:56:40 -0700
From:   Jacob Keller <jacob.e.keller@...el.com>
To:     Jakub Kicinski <kuba@...nel.org>, davem@...emloft.net
Cc:     jiri@...nulli.us, netdev@...r.kernel.org, kernel-team@...com
Subject: Re: [PATCH net-next] devlink: refactor end checks in
 devlink_nl_cmd_region_read_dumpit



On 5/13/2020 10:28 AM, Jakub Kicinski wrote:
> Clean up after recent fixes, move address calculations
> around and change the variable init, so that we can have
> just one start_offset == end_offset check.
> 
> Make the check a little stricter to preserve the -EINVAL
> error if requested start offset is larger than the region
> itself.
> 
> Signed-off-by: Jakub Kicinski <kuba@...nel.org>
> ---
>  net/core/devlink.c                            | 41 ++++++++-----------
>  .../drivers/net/netdevsim/devlink.sh          | 15 +++++++
>  2 files changed, 31 insertions(+), 25 deletions(-)
> 
> diff --git a/net/core/devlink.c b/net/core/devlink.c
> index 20f935fa29f5..7b76e5fffc10 100644
> --- a/net/core/devlink.c
> +++ b/net/core/devlink.c
> @@ -4215,7 +4215,6 @@ static int devlink_nl_region_read_snapshot_fill(struct sk_buff *skb,
>  						struct nlattr **attrs,
>  						u64 start_offset,
>  						u64 end_offset,
> -						bool dump,
>  						u64 *new_offset)
>  {
>  	struct devlink_snapshot *snapshot;
> @@ -4230,9 +4229,6 @@ static int devlink_nl_region_read_snapshot_fill(struct sk_buff *skb,
>  	if (!snapshot)
>  		return -EINVAL;
>  
> -	if (end_offset > region->size || dump)
> -		end_offset = region->size;
> -

Yea I saw this back when I was looking at enabling region dump without a
snapshot. At this point, it doesn't seem necessary, because the snapshot
time is relatively low, and the changes to make snapshot id a bit easier
to use in scripts (i.e. dynamic generation and saving) mean that it
isn't that useful.

Good to see this cleaned up!

>  	while (curr_offset < end_offset) {
>  		u32 data_size;
>  		u8 *data;
> @@ -4260,13 +4256,12 @@ static int devlink_nl_cmd_region_read_dumpit(struct sk_buff *skb,
>  					     struct netlink_callback *cb)
>  {
>  	const struct genl_dumpit_info *info = genl_dumpit_info(cb);
> -	u64 ret_offset, start_offset, end_offset = 0;
> +	u64 ret_offset, start_offset, end_offset = U64_MAX;
>  	struct nlattr **attrs = info->attrs;
>  	struct devlink_region *region;
>  	struct nlattr *chunks_attr;
>  	const char *region_name;
>  	struct devlink *devlink;
> -	bool dump = true;
>  	void *hdr;
>  	int err;
>  
> @@ -4294,8 +4289,21 @@ static int devlink_nl_cmd_region_read_dumpit(struct sk_buff *skb,
>  		goto out_unlock;
>  	}
>  
> +	if (attrs[DEVLINK_ATTR_REGION_CHUNK_ADDR] &&
> +	    attrs[DEVLINK_ATTR_REGION_CHUNK_LEN]) {
> +		if (!start_offset)
> +			start_offset =
> +				nla_get_u64(attrs[DEVLINK_ATTR_REGION_CHUNK_ADDR]);
> +
> +		end_offset = nla_get_u64(attrs[DEVLINK_ATTR_REGION_CHUNK_ADDR]);

At  first, reading this confused me a bit, but it makes sense. The end
is always "beginning + length".

If the start_offset is set before, this simply means that we needed to
read over multiple buffers. Ok.

> +		end_offset += nla_get_u64(attrs[DEVLINK_ATTR_REGION_CHUNK_LEN]);
> +	}
> +
> +	if (end_offset > region->size)
> +		end_offset = region->size;
> +
>  	/* return 0 if there is no further data to read */
> -	if (start_offset >= region->size) {
> +	if (start_offset == end_offset) {

Why change this to ==? isn't it possible to specify a start_offset that
is out of bounds? I think this should still be >=

>  		err = 0;
>  		goto out_unlock;
>  	}
> @@ -4322,27 +4330,10 @@ static int devlink_nl_cmd_region_read_dumpit(struct sk_buff *skb,
>  		goto nla_put_failure;
>  	}
>  
> -	if (attrs[DEVLINK_ATTR_REGION_CHUNK_ADDR] &&
> -	    attrs[DEVLINK_ATTR_REGION_CHUNK_LEN]) {
> -		if (!start_offset)
> -			start_offset =
> -				nla_get_u64(attrs[DEVLINK_ATTR_REGION_CHUNK_ADDR]);
> -
> -		end_offset = nla_get_u64(attrs[DEVLINK_ATTR_REGION_CHUNK_ADDR]);
> -		end_offset += nla_get_u64(attrs[DEVLINK_ATTR_REGION_CHUNK_LEN]);
> -		dump = false;
> -
> -		if (start_offset == end_offset) {
> -			err = 0;
> -			goto nla_put_failure;
> -		}
> -	}
> -
>  	err = devlink_nl_region_read_snapshot_fill(skb, devlink,
>  						   region, attrs,
>  						   start_offset,
> -						   end_offset, dump,
> -						   &ret_offset);
> +						   end_offset, &ret_offset);
>  
>  	if (err && err != -EMSGSIZE)
>  		goto nla_put_failure;
> diff --git a/tools/testing/selftests/drivers/net/netdevsim/devlink.sh b/tools/testing/selftests/drivers/net/netdevsim/devlink.sh
> index ad539eccddcb..de4b32fc4223 100755
> --- a/tools/testing/selftests/drivers/net/netdevsim/devlink.sh
> +++ b/tools/testing/selftests/drivers/net/netdevsim/devlink.sh
> @@ -146,6 +146,21 @@ regions_test()
>  
>  	check_region_snapshot_count dummy post-first-request 3
>  
> +	devlink region dump $DL_HANDLE/dummy snapshot 25 >> /dev/null
> +	check_err $? "Failed to dump snapshot with id 25"
> +
> +	devlink region read $DL_HANDLE/dummy snapshot 25 addr 0 len 1 >> /dev/null
> +	check_err $? "Failed to read snapshot with id 25 (1 byte)"
> +
> +	devlink region read $DL_HANDLE/dummy snapshot 25 addr 128 len 128 >> /dev/null
> +	check_err $? "Failed to read snapshot with id 25 (128 bytes)"
> +
> +	devlink region read $DL_HANDLE/dummy snapshot 25 addr 128 len $((1<<32)) >> /dev/null
> +	check_err $? "Failed to read snapshot with id 25 (oversized)"
> +
> +	devlink region read $DL_HANDLE/dummy snapshot 25 addr $((1<<32)) len 128 >> /dev/null 2>&1
> +	check_fail $? "Bad read of snapshot with id 25 did not fail"
> +
>  	devlink region del $DL_HANDLE/dummy snapshot 25
>  	check_err $? "Failed to delete snapshot with id 25"
>  
> 

Powered by blists - more mailing lists