linux-kernel - Re: [Patch 3/4] ipc/mqueue: strengthen checks on mqueue creation

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:	Tue, 01 May 2012 16:11:31 -0400
From:	Doug Ledford <dledford@...hat.com>
To:	KOSAKI Motohiro <kosaki.motohiro@...il.com>
CC:	linux-kernel@...r.kernel.org, akpm@...ux-foundation.org,
	sfr@...b.auug.org.au
Subject: Re: [Patch 3/4] ipc/mqueue: strengthen checks on mqueue creation

On 05/01/2012 04:01 PM, KOSAKI Motohiro wrote:
> (5/1/12 1:50 PM), Doug Ledford wrote:
>> We already check the mq attr struct if it's passed in, but now that the
>> admin can set system wide defaults separate from maximums, it's actually
>> possible to set the defaults to something that would overflow.  So,
>> if there is no attr struct passed in to the open call, check the default
>> values.
>>
>> While we are at it, simplify mq_attr_ok() by making it return 0 or an
>> error condition, so that way if we add more tests to it later, we have
>> the option of what error should be returned instead of the calling
>> location having to pick a possibly inaccurate error code.
>>
>> Signed-off-by: Doug Ledford<dledford@...hat.com>
>> ---
>>   ipc/mqueue.c |   27 ++++++++++++++++++---------
>>   1 files changed, 18 insertions(+), 9 deletions(-)
>>
>> diff --git a/ipc/mqueue.c b/ipc/mqueue.c
>> index 4b2892e..6089f73 100644
>> --- a/ipc/mqueue.c
>> +++ b/ipc/mqueue.c
>> @@ -673,27 +673,27 @@ static int mq_attr_ok(struct ipc_namespace *ipc_ns, struct mq_attr *attr)
>>   	int mq_treesize;
>>
>>   	if (attr->mq_maxmsg<= 0 || attr->mq_msgsize<= 0)
>> -		return 0;
>> +		return -EINVAL;
>>   	if (capable(CAP_SYS_RESOURCE)) {
>>   		if (attr->mq_maxmsg>  HARD_MSGMAX ||
>>   		    attr->mq_msgsize>  HARD_MSGSIZEMAX)
>> -			return 0;
>> +			return -EINVAL;
>>   	} else {
>>   		if (attr->mq_maxmsg>  ipc_ns->mq_msg_max ||
>>   				attr->mq_msgsize>  ipc_ns->mq_msgsize_max)
>> -			return 0;
>> +			return -EINVAL;
>>   	}
>>   	/* check for overflow */
>>   	if (attr->mq_msgsize>  ULONG_MAX/attr->mq_maxmsg)
>> -		return 0;
>> +		return -ENOMEM;
>>   	mq_treesize = attr->mq_maxmsg * sizeof(struct msg_msg) +
>>   		min_t(unsigned int, attr->mq_maxmsg, MQ_PRIO_MAX) *
>>   		sizeof(struct posix_msg_tree_node);
>>   	if ((unsigned long)(attr->mq_maxmsg * attr->mq_msgsize +
>>   			    mq_treesize)<
>>   	(unsigned long)(attr->mq_maxmsg * attr->mq_msgsize))
>> -		return 0;
>> -	return 1;
>> +		return -ENOMEM;
>> +	return 0;
> 
> But ENOMEM is more inaccurate. It almostly is used for kmalloc failure.

I chose ENOMEM for that particular error because above there we have
checked the passed in arguments to make sure that they don't violate our
allowances for max message or max message size.  If we violate either of
those items, we return EINVAL.  In this case, neither of the values is
invalid, it's just that together they make an overly large allocation.
I would see that as more helpful to a programmer than EINVAL when the
values are within the maximums allowed.  At least with ENOMEM the
programmer knows they have to reduce their combined message size and
message count in order to get things working.


-- 
Doug Ledford <dledford@...hat.com>
              GPG KeyID: 0E572FDD
	      http://people.redhat.com/dledford



Download attachment "signature.asc" of type "application/pgp-signature" (901 bytes)