[KIO-MTP] Fix null pointer dereference
ClosedPublic

Authored by feverfew on Apr 3 2020, 11:09 AM.

Details

Summary

A null pointer can be returned from getDevice() if a device is disconnected.
Passing NULL into LIBMTP_Get_Storage() results in a NULL pointer dereference.

BUG: 405838

Test Plan

Compiles. I couldn't reproduce this as described in the bug report, but from
reading the attached stacktrace in the bug report it's obvious what went wrong
here.

Diff Detail

Repository
R320 KIO Extras
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
feverfew created this revision.Apr 3 2020, 11:09 AM
Restricted Application added projects: Dolphin, Frameworks. · View Herald TranscriptApr 3 2020, 11:09 AM
Restricted Application added subscribers: kfm-devel, kde-frameworks-devel. · View Herald Transcript
feverfew requested review of this revision.Apr 3 2020, 11:09 AM

You're right about bug report, but it can fail in any other place, just in particular version it happen in updateStorageInfo Can we cache getDevice in m_device (in constructor) then use it everywhere. I think libmtp has guard against disconnected device and will not crash.

fvogt added a comment.Apr 3 2020, 11:40 AM

You're right about bug report, but it can fail in any other place, just in particular version it happen in updateStorageInfo Can we cache getDevice in m_device (in constructor) then use it everywhere. I think libmtp has guard against disconnected device and will not crash.

MTPDevice already does that.

If getDevice() returns nullptr, this means that MTPDevice::getDevice() returns nullptr. This can only happen if m_mtpdevice is nullptr, which will crash in MTPDevice::~MTPDevice sooner or later anyway.

So this patch will at most just delay the crash.

AFAICT MTPDevice is supposed to be destroyed in KMTPd::deviceRemoved on disconnection, but this is obviously racy.

If getDevice() returns nullptr, this means that MTPDevice::getDevice() returns nullptr. This can only happen if m_mtpdevice is nullptr, which will crash in MTPDevice::~MTPDevice sooner or later anyway.

I don't think so, libmtp knows device is not available then LIBMTP_Release_Device (invalid device) will not crash. So i prefer to cache device in storage as well so libmtp will just return false on invalid device.

fvogt added a comment.Apr 3 2020, 12:04 PM

There is no such thing as an "invalid device" at that point anymore. There's only nullptr.

LIBMTP_mtpdevice_t *MTPDevice::getDevice()
{
    if (!m_mtpdevice->storage) {
        qCDebug(LOG_KIOD_KMTPD) << "no storage found: reopen mtpdevice";
        LIBMTP_Release_Device(m_mtpdevice);
        m_mtpdevice = LIBMTP_Open_Raw_Device_Uncached(&m_rawdevice);
    }

    return m_mtpdevice;
}

What you're suggesting is to change MTPDevice::getDevice to return the old device if reopening fails - but reopening without releasing might not work.

I see we don't speak in same language :)
LIBMTP_Open_Raw_Device_Uncached(&m_rawdevice);
returns nullptr that's normal since device is inaccessible, i mean it does not need to call LIBMTP_Release_Device using m_mtpdevice is safe it's not nullptr, it's just a disconnected device and libmtp knows that.

fvogt added a comment.Apr 3 2020, 12:27 PM

I see we don't speak in same language :)
LIBMTP_Open_Raw_Device_Uncached(&m_rawdevice);
returns nullptr that's normal since device is inaccessible, i mean it does not need to call LIBMTP_Release_Device using m_mtpdevice is safe it's not nullptr, it's just a disconnected device and libmtp knows that.

Yes, and until that point everything is fine.
Only after m_mtpdevice = LIBMTP_Open_Raw_Device_Uncached(&m_rawdevice);, which sets m_mtpdevice to nullptr it goes down the path I outlined.

What you're suggesting is to change MTPDevice::getDevice to return the old device if reopening fails - but reopening without releasing might not work.

This seems to be a robust solution IMO, why do you suspect this might not work?

fvogt added a comment.Apr 3 2020, 12:43 PM

What you're suggesting is to change MTPDevice::getDevice to return the old device if reopening fails - but reopening without releasing might not work.

This seems to be a robust solution IMO, why do you suspect this might not work?

Because there can only be a single libusb session per device. So you have to release the old one before opening again.

@feverfew are you gonna try what i'm writing about or i should do it? Just use cached device, do not reopen since it'll return nullptr.

feverfew added a comment.EditedApr 3 2020, 1:16 PM

If you want to, feel free, I'm a bit tight on time. But I will say this. the whole getDevice() function confuses me. I'm not entirely sure why this if check is necessary at all. The lifetime of devices and its children (i.e storage) should be managed by the KMTPD daemon AFAICT. A device shouldn't be trying to re-open itself anywhere IMO. To me the getDevice() function should simply be a simple return to avoid this NULL issue happening. Even if a device doesn't exist anymore, no segfaults should happen when passing an "invalid" LIBMTP_mtpdevice_t to any other LIBMTP function?

So to be succinct, the only correct fix here is to change getDevice() to return m_mtpdevice?

anthonyfieroni added a comment.EditedApr 3 2020, 1:20 PM

So to be succinct, the only correct fix here is to change getDevice() to return m_mtpdevice?

Yes, then check if it's crash, in all other places LIBMTP_xxx should take care of and return false or nullptr depend of function returning value.

feverfew added a comment.EditedApr 3 2020, 2:11 PM

So to be succinct, the only correct fix here is to change getDevice() to return m_mtpdevice?

Yes, then check if it's crash, in all other places LIBMTP_xxx should take care of and return false or nullptr depend of function returning value.

What do you mean by "then check if it's crash"? Surely we should do nothing and let LIBMTP sort out printing errors and stuff? Eventually the device should be removed by the daemon anyway via the help of Solid::deviceRemoved(), and hence LIBMTP_Release_Device will be called once and in the correct place. I agree with everything after the comma though, LIBMTP_mtpdevice_t * should never be freed unless we explicitly do so ourselves.

fvogt added a comment.EditedApr 3 2020, 3:35 PM

I assume there is a reason why MTPDevice::getDevice() has code for handling this very specific case, so I wouldn't just remove it without figuring out why: https://i.redd.it/hfnl7xo8yovy.gif

If not, that would indeed be the best option.

feverfew updated this revision to Diff 79252.Apr 3 2020, 9:29 PM
  • Don't try to release device in get method

Ok so I've updated the code as can be seen. Mucked around with disconnecting/connecting, no issues on my side at least.

Ok that's look good to me. We can move that code in constructor just before loop for storages, then use m_mtpdevice instead of raw device, but it looks like removed code is just a noise and it shouldn't present at all.

+1

elvisangelaccio accepted this revision.Apr 5 2020, 3:49 PM

I assume there is a reason why MTPDevice::getDevice() has code for handling this very specific case, so I wouldn't just remove it without figuring out why: https://i.redd.it/hfnl7xo8yovy.gif

If not, that would indeed be the best option.

Unfortunately git blame doesn't seem to help us here.

I suggest to push this fix to master only and see what happens.

This revision is now accepted and ready to land.Apr 5 2020, 3:49 PM

@elvisangelaccio this peace of code is purely wrong at least m_storages is not updated to new device and not only. This code should never exists or try to hide some other bug.

feverfew added a comment.EditedApr 5 2020, 4:48 PM

I assume there is a reason why MTPDevice::getDevice() has code for handling this very specific case, so I wouldn't just remove it without figuring out why: https://i.redd.it/hfnl7xo8yovy.gif

If not, that would indeed be the best option.

Unfortunately git blame doesn't seem to help us here.

I suggest to push this fix to master only and see what happens.

By pushing this to master would we still be able to throw it up to 20.04.* if we decide it's stable enough? (also need to know to know what to put down as the FIXED-IN in the commit message)?

I assume there is a reason why MTPDevice::getDevice() has code for handling this very specific case, so I wouldn't just remove it without figuring out why: https://i.redd.it/hfnl7xo8yovy.gif

If not, that would indeed be the best option.

Unfortunately git blame doesn't seem to help us here.

I suggest to push this fix to master only and see what happens.

By pushing this to master would we still be able to throw it up to 20.04.* if we decide it's stable enough? (also need to know to know what to put down as the FIXED-IN in the commit message)?

@elvisangelaccio ping seeming as 20.04 release is close...

I assume there is a reason why MTPDevice::getDevice() has code for handling this very specific case, so I wouldn't just remove it without figuring out why: https://i.redd.it/hfnl7xo8yovy.gif

If not, that would indeed be the best option.

Unfortunately git blame doesn't seem to help us here.

I suggest to push this fix to master only and see what happens.

By pushing this to master would we still be able to throw it up to 20.04.* if we decide it's stable enough? (also need to know to know what to put down as the FIXED-IN in the commit message)?

Sure, we can cherry-pick that commit later.

This revision was automatically updated to reflect the committed changes.