linux-kernel - Re: [Openipmi-developer] [PATCH v2] ipmi: Fix kernel panic at ipmi_ssif

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: <5cd886a8-5fa8-2828-ce48-c7d507b6b571@acm.org>
Date:   Tue, 28 Mar 2017 07:18:40 -0500
From:   Corey Minyard <minyard@....org>
To:     joechang@...eaurora.org
Cc:     anjiandi@...eaurora.org, openipmi-developer@...ts.sourceforge.net,
        linux-kernel@...r.kernel.org, Corey Minyard <tcminyard@...il.com>
Subject: Re: [Openipmi-developer] [PATCH v2] ipmi: Fix kernel panic at
 ipmi_ssif_thread()

On 03/27/2017 09:29 PM, joechang@...eaurora.org wrote:
> Hi Corey,
>
> I really appreciate your patience and detail code review.

Not a problem at all, thanks for sticking with this.

This will be queued for the next release, and I'll request backports to 
stable trees.

Thanks,

-corey

> I re-generate new patch and send new patch to you.
> Please feel free to feedback to me if something need to modify from the
> new patch..
>
> Thank you,
> Joseph.
>
> Corey Minyard 於 2017-03-27 20:32 寫到:
>> On 03/27/2017 02:31 AM, Joeseph Chang wrote:
>>> From: Joeseph Chang <joechang@...eaurora.org>
>>>
>>> Since patch v1 touch ssif_info->multi_pos and ssif_info->multi_data
>>> after ssif_i2c_send(). msg_written_handler can be called at any time
>>> after ssif_i2c_send(). There is possible to have concurrent access to
>>> ssif_info->multi_pos or ssif_info->multi_data at msg_written_handler.
>>>
>>> Revert patch v1 and add new local pointer "i2c_data" which point to
>>> i2c data and size that will be sent out to avoid concurrent access in
>>> msg_written_handler().
>>>
>>> Signed-off-by: Joeseph Chang <joechang@...eaurora.org>
>>> ---
>>>    drivers/char/ipmi/ipmi_ssif.c | 24 ++++++++++++------------
>>>    1 file changed, 12 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/drivers/char/ipmi/ipmi_ssif.c
>>> b/drivers/char/ipmi/ipmi_ssif.c
>>> index 39346ee..f36f018 100644
>>> --- a/drivers/char/ipmi/ipmi_ssif.c
>>> +++ b/drivers/char/ipmi/ipmi_ssif.c
>>> @@ -852,6 +852,7 @@ static void msg_written_handler(struct ssif_info
>>> *ssif_info, int result,
>>>    				unsigned char *data, unsigned int len)
>>>    {
>>>    	int rv;
>>> +	unsigned char *i2c_data;
>> The variable should be in the block where it is used.  The basic rule
>> is to limit scope as much as possible.
>>
>> The name is also a little vague, though not terrible.
>>
>>>      	/* We are single-threaded here, so no need for a lock. */
>>>    	if (result < 0) {
>>> @@ -899,13 +900,22 @@ static void msg_written_handler(struct ssif_info
>>> *ssif_info, int result,
>>>    			left = 32;
>>>    		/* Length byte. */
>>>    		ssif_info->multi_data[ssif_info->multi_pos] = left;
>>> +		i2c_data = ssif_info->multi_data + ssif_info->multi_pos;
>>> +		ssif_info->multi_pos += left;
>>> +		if (left < 32)
>>> +			/*
>>> +			 * Write is finished.  Note that we must end
>>> +			 * with a write of less than 32 bytes to
>>> +			 * complete the transaction, even if it is
>>> +			 * zero bytes.
>>> +			 */
>>> +			ssif_info->multi_data = NULL;
>> Can you make this change not relative to the previous change?  Just do
>> a "git rebase -i ...." and squash the
>> changes.
>>
>> I saw your previous emails, but I can't take those because of the
>> comments at the top and the text from
>> the reply.  You can add comments to a submission that don't go into
>> git, just put them after the '---" in
>> the header.
>>
>> Thanks,
>>
>> -corey
>>
>>>      		rv = ssif_i2c_send(ssif_info, msg_written_handler,
>>>    				  I2C_SMBUS_WRITE,
>>>    				  SSIF_IPMI_MULTI_PART_REQUEST_MIDDLE,
>>> -				  ssif_info->multi_data + ssif_info->multi_pos,
>>> +				  i2c_data,
>>>    				  I2C_SMBUS_BLOCK_DATA);
>>> -
>>>    		if (rv < 0) {
>>>    			/* request failed, just return the error. */
>>>    			ssif_inc_stat(ssif_info, send_errors);
>>> @@ -914,16 +924,6 @@ static void msg_written_handler(struct ssif_info
>>> *ssif_info, int result,
>>>    				pr_info("Error from i2c_non_blocking_op(3)\n");
>>>    			msg_done_handler(ssif_info, -EIO, NULL, 0);
>>>    		}
>>> -
>>> -		ssif_info->multi_pos += left;
>>> -		if (left < 32)
>>> -			/*
>>> -			 * Write is finished.  Note that we must end
>>> -			 * with a write of less than 32 bytes to
>>> -			 * complete the transaction, even if it is
>>> -			 * zero bytes.
>>> -			 */
>>> -			ssif_info->multi_data = NULL;
>>>    	} else {
>>>    		/* Ready to request the result. */
>>>    		unsigned long oflags, *flags;
> ------------------------------------------------------------------------------
> Check out the vibrant tech community on one of the world's most
> engaging tech sites, Slashdot.org! http://sdm.link/slashdot
> _______________________________________________
> Openipmi-developer mailing list
> Openipmi-developer@...ts.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/openipmi-developer