libkmod: Fix open file which some char device

[opsiff]

use lseek SEEK_CUR to get whether it is a regular file, lseek return -ESPIPE when fd is associated with a pipe, socket, or FIFO.

commit 883d931 upstream ("modprobe: Allow passing path to module") allow to modprobe a file, but not handle opening a FIFO device such as modprobe /dev/tty1.

commit 8d03b6c uptream ("libkmod: Use pread where appropriate") fix some cases, such as open /dev/tty1, but not for /dev/vmnet0 or /dev/userio etc.

Can be reproduced in run lshw in root and install some vm such ad vmware, more about the case are in the Link.

Link: lyonel/lshw#110
Link: https://bugs.launchpad.net/ubuntu/+source/lshw/+bug/2069649
Link: https://bbs.deepin.org.cn/post/291466
Reported-by: Qi Xu xuqi@uniontech.com
Reported-by: lionheartyu dongshengyuan@uniontech.com
Fixes: 883d931 ("modprobe: Allow passing path to module")

[lucasdemarchi]

LGTM. @guludo since this is coming from that commit to allow modprobing from a relative path... are you ok with this too?

[codecov]

Codecov Report

❌ Patch coverage is 50.00000% with 1 line in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
libkmod/libkmod-file.c	50.00%	0 Missing and 1 partial ⚠️

Files with missing lines	Coverage Δ
libkmod/libkmod-file.c	75.71% <50.00%> (ø)

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[evelikov]

General comment, take it with a pinch of salt.

Having the lseek without any comment, might result in us removing it a few month/years down the line.

Alternative is to use fstat + S_IFREG, which will also catch character/block/directory/symlink... and reduce TOCTOU races. Admittedly I'm not 100% sure:

• which syscall is lighter
• if any existing path intentionally does not resolve when path is a symlink

[opsiff]

Having the lseek without any comment, might result in us removing it a few month/years down the line.

OK, I will add a comment.

Alternative is to use fstat + S_IFREG, which will also catch character/block/directory/symlink... and reduce TOCTOU races.

lseek also use file->fd, I use lseek because origin it have a lseek call before the pr here.
#189

• which syscall is lighter

I think lseek is lighter little, I have no idea here.

• if any existing path intentionally does not resolve when path is a symlink

I think symlink resolve in open, I am also not 100% sure.

BRs
Wentao Guan

[opsiff]

Here is fstat version patch, it is longer than lseek, @evelikov :

diff --git a/libkmod/libkmod-file.c b/libkmod/libkmod-file.c
index d651fa2..484a13d 100644
--- a/libkmod/libkmod-file.c
+++ b/libkmod/libkmod-file.c
@@ -78,6 +78,7 @@ int kmod_file_open(const struct kmod_ctx *ctx, const char *filename,
                   struct kmod_file **out_file)
 {
        _cleanup_free_ struct kmod_file *file;
+       struct stat st;
        char buf[7];
        ssize_t sz;
        int ret;
@@ -96,6 +97,12 @@ int kmod_file_open(const struct kmod_ctx *ctx, const char *filename,
        if (file->fd < 0)
                return -errno;

+       if (fstat(file->fd, &st) < 0)
+               return -errno;
+       /* make sure it is a regular file */
+       if (!S_ISREG(st.st_mode))
+               return -EINVAL;
+
        sz = pread_str_safe(file->fd, buf, sizeof(buf), 0);
        if (sz != (sizeof(buf) - 1)) {
                if (sz < 0)

[evelikov]

The TOCTOU mentioned refers to the time frame between the file/path checks and the respective open() call. Mind you, it is pre-existing - assuming it's a genuine concern - and not something to worry for this PR.

I think symlink resolve in open, I am also not 100% sure.

Correct - symlinks are not resolved, only when using open(... O_NOFOLLOW ...). I remember seeing symlinks which lack the read permission - but seemingly that's macOS specific and not POSIX compliant.

[lucasdemarchi]

Having the lseek without any comment, might result in us removing it a few month/years down the line.

right, I think we should just add a comment. To me it makes sense to bail out on non-seekable fd since this function depends on lseek to work.

[lucasdemarchi]

with the comment added this seems good to me. @evelikov does that handle your concern?

[lucasdemarchi]

Applied, thanks